This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]