[PATCH] Fix readlink calls in GDB

[PATCH] Fix readlink calls in GDB

[Pedro Alves]

This is largely based on a patch Maxime sent me, to fix readlink calls
in GDB.

Several readlink calls in gdb are wrong.  readlink doesn't append the
terminating nul, so if we're going to need to do that, we need to pass
'sizeof (buf) - 1' as buffer size.

See:

https://www.securecoding.cert.org/confluence/display/seccode/POS30-C.+Use+the+readlink%28%29+function+properly

Tested on x86_64 Fedora 17, and checked in.

gdb/
2012-11-26  Maxime Villard  <rustyBSD@gmx.fr>
	    Pedro Alves  <palves@redhat.com>

	* common/linux-osdata.c (linux_xfer_osdata_fds): Decrease buffer
	size parameter passed to readlink by one byte.
	* fbsd-nat.c (fbsd_pid_to_exec_file): Ditto.
	* linux-nat.c (linux_child_pid_to_exec_file): Ditto.
	* nbsd-nat.c (nbsd_pid_to_exec_file): Ditto.
	* inf-child.c (inf_child_fileio_readlink): Decrease local buffer's
	size by one byte.

gdb/gdbserver/
2012-11-26  Maxime Villard  <rustyBSD@gmx.fr>

	* hostio.c (handle_readlink): Decrease buffer size
	parameter passed to readlink by one byte.
---
 gdb/common/linux-osdata.c |    2 +-
 gdb/fbsd-nat.c            |    2 +-
 gdb/gdbserver/hostio.c    |    2 +-
 gdb/inf-child.c           |    2 +-
 gdb/linux-nat.c           |    2 +-
 gdb/nbsd-nat.c            |    2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/common/linux-osdata.c b/gdb/common/linux-osdata.c
index d54f9d3..b275495 100644
--- a/gdb/common/linux-osdata.c
+++ b/gdb/common/linux-osdata.c
@@ -737,7 +737,7 @@ linux_xfer_osdata_fds (gdb_byte *readbuf,
 			    continue;
 
 			  fdname = xstrprintf ("%s/%s", pathname, dp2->d_name);
-			  rslt = readlink (fdname, buf, 1000);
+			  rslt = readlink (fdname, buf, sizeof (buf) - 1);
 			  if (rslt >= 0)
 			    buf[rslt] = '\0';
 
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 254a01a..5eaecdd 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -55,7 +55,7 @@ fbsd_pid_to_exec_file (int pid)
 #endif
 
   path = xstrprintf ("/proc/%d/file", pid);
-  if (readlink (path, buf, MAXPATHLEN) == -1)
+  if (readlink (path, buf, MAXPATHLEN - 1) == -1)
     {
       xfree (buf);
       buf = NULL;
diff --git a/gdb/gdbserver/hostio.c b/gdb/gdbserver/hostio.c
index 72e334c..e89e100 100644
--- a/gdb/gdbserver/hostio.c
+++ b/gdb/gdbserver/hostio.c
@@ -483,7 +483,7 @@ handle_readlink (char *own_buf, int *new_packet_len)
       return;
     }
 
-  ret = readlink (filename, linkname, sizeof linkname);
+  ret = readlink (filename, linkname, sizeof (linkname) - 1);
   if (ret == -1)
     {
       hostio_error (own_buf);
diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index ae2dd1e..3530e75 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -346,7 +346,7 @@ inf_child_fileio_readlink (const char *filename, int *target_errno)
   /* We support readlink only on systems that also provide a compile-time
      maximum path length (MAXPATHLEN), at least for now.  */
 #if defined (HAVE_READLINK) && defined (MAXPATHLEN)
-  char buf[MAXPATHLEN];
+  char buf[MAXPATHLEN - 1];
   int len;
   char *ret;
 
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 45f7e24..f5ca977 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4319,7 +4319,7 @@ linux_child_pid_to_exec_file (int pid)
   memset (name2, 0, MAXPATHLEN);
 
   sprintf (name1, "/proc/%d/exe", pid);
-  if (readlink (name1, name2, MAXPATHLEN) > 0)
+  if (readlink (name1, name2, MAXPATHLEN - 1) > 0)
     return name2;
   else
     return name1;
diff --git a/gdb/nbsd-nat.c b/gdb/nbsd-nat.c
index 14b562f..7f5df66 100644
--- a/gdb/nbsd-nat.c
+++ b/gdb/nbsd-nat.c
@@ -34,7 +34,7 @@ nbsd_pid_to_exec_file (int pid)
   char *path;
 
   path = xstrprintf ("/proc/%d/exe", pid);
-  if (readlink (path, buf, MAXPATHLEN) == -1)
+  if (readlink (path, buf, MAXPATHLEN - 1) == -1)
     {
       xfree (buf);
       buf = NULL;

RE: [PATCH] Fix readlink calls in GDB

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : lundi 26 novembre 2012 15:21
> À : gdb-patches@sourceware.org
> Cc : rustyBSD@gmx.fr
> Objet : [PATCH] Fix readlink calls in GDB
> 
> This is largely based on a patch Maxime sent me, to fix readlink calls
> in GDB.
> 
> Several readlink calls in gdb are wrong.  readlink doesn't append the
> terminating nul, so if we're going to need to do that, we need to pass
> 'sizeof (buf) - 1' as buffer size.

  Just a question:
your change doesn't seem to add the terminating '\0'
in all the calls concerned, is this because it concerns specific 
native files for operating systems that already append the '\0' at the end?

  Because most arguments are used a 'char *'
and thus the terminating zero is required, no?


Pierre Muller

Re: [PATCH] Fix readlink calls in GDB

[Pedro Alves]

On 11/26/2012 02:42 PM, Pierre Muller wrote:

>   Just a question:
> your change doesn't seem to add the terminating '\0'
> in all the calls concerned, is this because it concerns specific 
> native files for operating systems that already append the '\0' at the end?

No.  That must be done in all operating systems.  If you look at the code
that surrounds the lines touched by the patch, you'll find the preexisting
code doing that.


-- 
Pedro Alves

RE: [PATCH] Fix readlink calls in GDB

[Pierre Muller]

  I don't see any code setting the terminal char to '\0'
in fbsd_pid_to_exec_file (fbsd-nat.c:40 rev 1.29)
nor in linux_child_pid_to_exec_file (linux-nat.c:4311 rev 1.261)
nor in nbsd_pid_to_exec_file (nbsd-nat.c:30 rev 1.10)...

  Maybe I am missing something...


Pierre


> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : lundi 26 novembre 2012 16:15
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org; rustyBSD@gmx.fr
> Objet : Re: [PATCH] Fix readlink calls in GDB
> 
> On 11/26/2012 02:42 PM, Pierre Muller wrote:
> 
> >   Just a question:
> > your change doesn't seem to add the terminating '\0'
> > in all the calls concerned, is this because it concerns specific
> > native files for operating systems that already append the '\0' at the
> end?
> 
> No.  That must be done in all operating systems.  If you look at the code
> that surrounds the lines touched by the patch, you'll find the preexisting
> code doing that.
> 
> 
> --
> Pedro Alves

Re: [PATCH] Fix readlink calls in GDB

[Pedro Alves]

On 11/26/2012 03:31 PM, Pierre Muller wrote:
>   I don't see any code setting the terminal char to '\0'
> in fbsd_pid_to_exec_file (fbsd-nat.c:40 rev 1.29)

char *
fbsd_pid_to_exec_file (int pid)
{
  size_t len = MAXPATHLEN;
  char *buf = xcalloc (len, sizeof (char));
              ^^^^^^^
  char *path;

xcalloc memsets the memory to zero before returning.  See man calloc.

> nor in linux_child_pid_to_exec_file (linux-nat.c:4311 rev 1.261)

static char *
linux_child_pid_to_exec_file (int pid)
{
  char *name1, *name2;

  name1 = xmalloc (MAXPATHLEN);
  name2 = xmalloc (MAXPATHLEN);
  make_cleanup (xfree, name1);
  make_cleanup (xfree, name2);
  memset (name2, 0, MAXPATHLEN);
  ^^^^^^

That memset clear the whole buffer, thus the buffer ends up
always nul-terminated.

> nor in nbsd_pid_to_exec_file (nbsd-nat.c:30 rev 1.10)...

That likewise uses xcalloc.

-- 
Pedro Alves

RE: [PATCH] Fix readlink calls in GDB

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : lundi 26 novembre 2012 16:45
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org; rustyBSD@gmx.fr
> Objet : Re: [PATCH] Fix readlink calls in GDB
> 
> On 11/26/2012 03:31 PM, Pierre Muller wrote:
> >   I don't see any code setting the terminal char to '\0'
> > in fbsd_pid_to_exec_file (fbsd-nat.c:40 rev 1.29)
> 
> char *
> fbsd_pid_to_exec_file (int pid)
> {
>   size_t len = MAXPATHLEN;
>   char *buf = xcalloc (len, sizeof (char));
>               ^^^^^^^
>   char *path;
> 
> xcalloc memsets the memory to zero before returning.  See man calloc.

  Thank you for the answers,
I am sorry to have wasted your time :(
  I didn't know calloc was zeroing data...
 
 
> > nor in linux_child_pid_to_exec_file (linux-nat.c:4311 rev 1.261)
> 
> static char *
> linux_child_pid_to_exec_file (int pid)
> {
>   char *name1, *name2;
> 
>   name1 = xmalloc (MAXPATHLEN);
>   name2 = xmalloc (MAXPATHLEN);
>   make_cleanup (xfree, name1);
>   make_cleanup (xfree, name2);
>   memset (name2, 0, MAXPATHLEN);
>   ^^^^^^
>
> That memset clear the whole buffer, thus the buffer ends up
> always nul-terminated.

  I completely missed that memset call :(


  Does that mean that
   name2 = xzalloc (MAXPATHLEN);
or
   name2 = xcalloc (MAXPATHLEN, 1);

could have replaced the two lines:
>   name2 = xmalloc (MAXPATHLEN);
>   memset (name2, 0, MAXPATHLEN);

  
Thank you again for taking the time to 
answer my questions...


Pierre Muller

Re: [PATCH] Fix readlink calls in GDB

[Pedro Alves]

On 11/26/2012 04:33 PM, Pierre Muller wrote:

>   Thank you for the answers,
> I am sorry to have wasted your time :(
>   I didn't know calloc was zeroing data...

No problem.

>   Does that mean that
>    name2 = xzalloc (MAXPATHLEN);
> or
>    name2 = xcalloc (MAXPATHLEN, 1);
>
> 
> could have replaced the two lines:
>>   name2 = xmalloc (MAXPATHLEN);
>>   memset (name2, 0, MAXPATHLEN);

Yep.

-- 
Pedro Alves