* [PATCH][PR gdb/16013] Fix off-by-one errors in *scanf format strings @ 2013-10-14 10:52 Gary Benson 2013-10-14 11:25 ` Pedro Alves 0 siblings, 1 reply; 4+ messages in thread From: Gary Benson @ 2013-10-14 10:52 UTC (permalink / raw) To: gdb-patches; +Cc: dcb314 Hi all, This patch fixes a number of off-by-one errors in *scanf format strings. Ok to commit? Thanks, Gary -- http://gbenson.net/ 2013-10-14 Gary Benson <gbenson@redhat.com> PR 16013 * common/linux-osdata.c (command_from_pid): Fix off-by-one error in fscanf format string. (print_sockets): Fix off-by-one error in sscanf format string. (linux_xfer_osdata_modules): Likewise. diff --git a/gdb/common/linux-osdata.c b/gdb/common/linux-osdata.c index 9723839..8ebbab8 100644 --- a/gdb/common/linux-osdata.c +++ b/gdb/common/linux-osdata.c @@ -137,7 +137,7 @@ command_from_pid (char *command, int maxlen, PID_T pid) (for the brackets). */ char cmd[32]; PID_T stat_pid; - int items_read = fscanf (fp, "%lld %32s", &stat_pid, cmd); + int items_read = fscanf (fp, "%lld %31s", &stat_pid, cmd); if (items_read == 2 && pid == stat_pid) { @@ -880,7 +880,7 @@ print_sockets (unsigned short family, int tcp, struct buffer *buffer) int result; result = sscanf (buf, - "%d: %33[0-9A-F]:%X %33[0-9A-F]:%X %X %X:%X %X:%lX %X %d %d %lu %512s\n", + "%d: %33[0-9A-F]:%X %33[0-9A-F]:%X %X %X:%X %X:%lX %X %d %d %lu %511s\n", &sl, local_address, &local_port, remote_address, &remote_port, @@ -1471,7 +1471,7 @@ linux_xfer_osdata_modules (gdb_byte *readbuf, int items_read; items_read = sscanf (buf, - "%64s %d %d %256s %16s 0x%llx", + "%63s %d %d %255s %15s 0x%llx", name, &size, &uses, dependencies, status, &address); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][PR gdb/16013] Fix off-by-one errors in *scanf format strings 2013-10-14 10:52 [PATCH][PR gdb/16013] Fix off-by-one errors in *scanf format strings Gary Benson @ 2013-10-14 11:25 ` Pedro Alves 2013-10-18 14:39 ` [PATCH v2][PR " Gary Benson 0 siblings, 1 reply; 4+ messages in thread From: Pedro Alves @ 2013-10-14 11:25 UTC (permalink / raw) To: gdb-patches, dcb314 On 10/14/2013 11:52 AM, Gary Benson wrote: > Hi all, > > This patch fixes a number of off-by-one errors in *scanf format > strings. These could be fixed by either reducing the length specified in the format string, or, by increasing the buffers. Either such change would be obvious from a coding perspective. But the part that requires a rationale, is, that one that justifies the taken approach. That will be governed what the actual lengths of these fields on the kernel side. E.g.: /* sizeof (cmd) should be greater or equal to TASK_COMM_LEN (in include/linux/sched.h in the Linux kernel sources) plus two (for the brackets). */ char cmd[32]; PID_T stat_pid; int items_read = fscanf (fp, "%lld %32s", &stat_pid, cmd); Did you check the value of TASK_COMM_LEN ? (I haven't). Same for the other fields. -- Pedro Alves ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings 2013-10-14 11:25 ` Pedro Alves @ 2013-10-18 14:39 ` Gary Benson 2013-10-18 16:38 ` Pedro Alves 0 siblings, 1 reply; 4+ messages in thread From: Gary Benson @ 2013-10-18 14:39 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, dcb314 Pedro Alves wrote: > These could be fixed by either reducing the length specified > in the format string, or, by increasing the buffers. Either > such change would be obvious from a coding perspective. But > the part that requires a rationale, is, that one that justifies > the taken approach. That will be governed what the actual lengths > of these fields on the kernel side. E.g.: > > /* sizeof (cmd) should be greater or equal to TASK_COMM_LEN (in > include/linux/sched.h in the Linux kernel sources) plus two > (for the brackets). */ > char cmd[32]; > PID_T stat_pid; > int items_read = fscanf (fp, "%lld %32s", &stat_pid, cmd); > > Did you check the value of TASK_COMM_LEN ? (I haven't). > > Same for the other fields. Ok, I think I have now checked *everything* :) In the first hunk, the maximum size is 16 (including the terminator). Add two for the brackets makes 18, so 32 is big enough. I don't know if there would be any benefit to reducing the buffer here to 18 (does it make a difference if things are declared to be a power of two in size?) For the second hunk I caused all the unused variables to be skipped, which took care of the buffer "extra" being too small. I also reduced the size specifiers of local_address and remote_address from 33 to 32. This is the correct size (for IPv6) but was not causing an overflow as NI_MAXHOST is 1025 on my machine. I added a compile-time check for this, but don't know if this is overkill. In the third hunk, the dependencies field could be arbitrarily long, so I've rewritten it using strtok. While doing this I noticed that size (an unsigned int) is parsed as "%d" but printed as "%u" so I changed the parsing to "%u". I left untouched the funky indentation of the lines following the buffer_xml_printf but could change them to something else if required. Is this ok? Thanks, Gary -- http://gbenson.net/ 2013-10-18 Gary Benson <gbenson@redhat.com> PR 16013 * common/linux-osdata.c (command_from_pid): Modified fscanf format string to avoid leaving cmd unterminated. (print_sockets): Do not parse tlen, inode, sl, timeout, txq, rxq, trun, retn or extra. (Avoids leaving extra unterminated.) Check that local_address and remote_address will not overflow. (linux_xfer_osdata_modules): Parse lines using strtok to avoid leaving dependencies unterminated. Parse size as "%u" to match definition. diff --git a/gdb/common/linux-osdata.c b/gdb/common/linux-osdata.c index 9723839..29c3d77 100644 --- a/gdb/common/linux-osdata.c +++ b/gdb/common/linux-osdata.c @@ -137,7 +137,7 @@ command_from_pid (char *command, int maxlen, PID_T pid) (for the brackets). */ char cmd[32]; PID_T stat_pid; - int items_read = fscanf (fp, "%lld %32s", &stat_pid, cmd); + int items_read = fscanf (fp, "%lld %31s", &stat_pid, cmd); if (items_read == 2 && pid == stat_pid) { @@ -871,29 +871,23 @@ print_sockets (unsigned short family, int tcp, struct buffer *buffer) if (fgets (buf, sizeof (buf), fp)) { uid_t uid; - unsigned long tlen, inode; - int sl, timeout; unsigned int local_port, remote_port, state; - unsigned int txq, rxq, trun, retn; char local_address[NI_MAXHOST], remote_address[NI_MAXHOST]; - char extra[512]; int result; +#if NI_MAXHOST <= 32 +#error "local_address and remote_address buffers too small" +#endif + result = sscanf (buf, - "%d: %33[0-9A-F]:%X %33[0-9A-F]:%X %X %X:%X %X:%lX %X %d %d %lu %512s\n", - &sl, + "%*d: %32[0-9A-F]:%X %32[0-9A-F]:%X %X %*X:%*X %*X:%*X %*X %d %*d %*u %*s\n", + local_address, &local_port, remote_address, &remote_port, &state, - &txq, &rxq, - &trun, &tlen, - &retn, - &uid, - &timeout, - &inode, - extra); + &uid); - if (result == 15) + if (result == 6) { union socket_addr locaddr, remaddr; size_t addr_size; @@ -1464,19 +1458,42 @@ linux_xfer_osdata_modules (gdb_byte *readbuf, { if (fgets (buf, sizeof (buf), fp)) { - char name[64], dependencies[256], status[16]; + char *name, *dependencies, *status, *tmp; unsigned int size; unsigned long long address; int uses; - int items_read; - - items_read = sscanf (buf, - "%64s %d %d %256s %16s 0x%llx", - name, &size, &uses, - dependencies, status, &address); - if (items_read == 6) - buffer_xml_printf ( + name = strtok (buf, " "); + if (name == NULL) + continue; + + tmp = strtok (NULL, " "); + if (tmp == NULL) + continue; + if (sscanf (tmp, "%u", &size) != 1) + continue; + + tmp = strtok (NULL, " "); + if (tmp == NULL) + continue; + if (sscanf (tmp, "%d", &uses) != 1) + continue; + + dependencies = strtok (NULL, " "); + if (dependencies == NULL) + continue; + + status = strtok (NULL, " "); + if (status == NULL) + continue; + + tmp = strtok (NULL, "\n"); + if (tmp == NULL) + continue; + if (sscanf (tmp, "%llx", &address) != 1) + continue; + + buffer_xml_printf ( &buffer, "<item>" "<column name=\"name\">%s</column>" ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings 2013-10-18 14:39 ` [PATCH v2][PR " Gary Benson @ 2013-10-18 16:38 ` Pedro Alves 0 siblings, 0 replies; 4+ messages in thread From: Pedro Alves @ 2013-10-18 16:38 UTC (permalink / raw) To: gdb-patches, dcb314 On 10/18/2013 03:39 PM, Gary Benson wrote: > Pedro Alves wrote: >> These could be fixed by either reducing the length specified >> in the format string, or, by increasing the buffers. Either >> such change would be obvious from a coding perspective. But >> the part that requires a rationale, is, that one that justifies >> the taken approach. That will be governed what the actual lengths >> of these fields on the kernel side. E.g.: >> >> /* sizeof (cmd) should be greater or equal to TASK_COMM_LEN (in >> include/linux/sched.h in the Linux kernel sources) plus two >> (for the brackets). */ >> char cmd[32]; >> PID_T stat_pid; >> int items_read = fscanf (fp, "%lld %32s", &stat_pid, cmd); >> >> Did you check the value of TASK_COMM_LEN ? (I haven't). >> >> Same for the other fields. > > Ok, I think I have now checked *everything* :) > > In the first hunk, the maximum size is 16 (including the terminator). > Add two for the brackets makes 18, so 32 is big enough. I don't know > if there would be any benefit to reducing the buffer here to 18 I think it'd be beneficial for readability. I'd prefer making it 18 then. > (does it make a difference if things are declared to be a power of two in > size?) I don't think so. In any case, this code is not really a hot path or that size sensitive. > For the second hunk I caused all the unused variables to be skipped, > which took care of the buffer "extra" being too small. I also reduced > the size specifiers of local_address and remote_address from 33 to 32. > This is the correct size (for IPv6) but was not causing an overflow as > NI_MAXHOST is 1025 on my machine. I added a compile-time check for > this, but don't know if this is overkill. That's fine. > > In the third hunk, the dependencies field could be arbitrarily long, > so I've rewritten it using strtok. While doing this I noticed that > size (an unsigned int) is parsed as "%d" but printed as "%u" so I > changed the parsing to "%u". I left untouched the funky indentation > of the lines following the buffer_xml_printf but could change them > to something else if required. > > Is this ok? > Yes, this looks excellent! Thanks a lot for doing all this. > result = sscanf (buf, > - "%d: %33[0-9A-F]:%X %33[0-9A-F]:%X %X %X:%X %X:%lX %X %d %d %lu %512s\n", > - &sl, > + "%*d: %32[0-9A-F]:%X %32[0-9A-F]:%X %X %*X:%*X %*X:%*X %*X %d %*d %*u %*s\n", > + Spurious newline? > local_address, &local_port, -- Pedro Alves ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-10-18 16:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-10-14 10:52 [PATCH][PR gdb/16013] Fix off-by-one errors in *scanf format strings Gary Benson 2013-10-14 11:25 ` Pedro Alves 2013-10-18 14:39 ` [PATCH v2][PR " Gary Benson 2013-10-18 16:38 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).