[PATCH][PR gdb/16013] Fix off-by-one errors in *scanf format strings

[PATCH][PR gdb/16013] Fix off-by-one errors in *scanf format strings

[Gary Benson]

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);
 

Re: [PATCH][PR gdb/16013] Fix off-by-one errors in *scanf format strings

[Pedro Alves]

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

[PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings

[Gary Benson]

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>"

Re: [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings

[Pedro Alves]

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