public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Gary Benson <gbenson@redhat.com>
To: Pedro Alves <palves@redhat.com>
Cc: gdb-patches@sourceware.org, dcb314@hotmail.com
Subject: [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings
Date: Fri, 18 Oct 2013 14:39:00 -0000	[thread overview]
Message-ID: <20131018143903.GA14055@blade.nx> (raw)
In-Reply-To: <525BD49B.4080700@redhat.com>

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

  reply	other threads:[~2013-10-18 14:39 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-14 10:52 [PATCH][PR " Gary Benson
2013-10-14 11:25 ` Pedro Alves
2013-10-18 14:39   ` Gary Benson [this message]
2013-10-18 16:38     ` [PATCH v2][PR " Pedro Alves

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131018143903.GA14055@blade.nx \
    --to=gbenson@redhat.com \
    --cc=dcb314@hotmail.com \
    --cc=gdb-patches@sourceware.org \
    --cc=palves@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).