Re: [PATCH 0/1] Fix internal warning when "gdb -p xxx"

[Pedro Alves <palves at redhat dot com>]

On 03/18/2014 07:37 AM, Hui Zhu wrote:
> On 03/18/14 00:56, Pedro Alves wrote:

> According to your comments,  I make a new patch that change all
> methods return a static buffer.

Thank you.

>> Bummer that we don't have a test that caught this.  :-(
>>
> 
> Yes, I found it when I did regression test.

WDYM?  What test failed then?

> Does testsuite have some example to test a GDB feature like "gdb -p"?

I guess we'd refactor attach.exp a little to test that in addition
to "attach", on native targets.  And I supposed we could pass down the
-p to gdb by tweaking GDBFLAGS, like several tests do.  gdb.base/corefile.exp
sounds like the model to follow, as that spawns "gdb -core", which is very
similar to "gdb -p".

> 2014-03-18  Hui Zhu  <hui@codesourcery.com>
> 
> 	* darwin-nat.c (darwin_pid_to_exec_file): Change xmalloc to
> 	static buffer.
> 	* 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.
> 
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -1991,12 +1991,9 @@ set_enable_mach_exceptions (char *args,
>   static char *
>   darwin_pid_to_exec_file (struct target_ops *self, int pid)
>   {
> -  char *path;
> +  static char path[PATH_MAX];
>     int res;
>   
> -  path = xmalloc (PATH_MAX);
> -  make_cleanup (xfree, path);
> -
>     res = proc_pidinfo (pid, PROC_PIDPATHINFO, 0, path, PATH_MAX);
>     if (res >= 0)
>       return path;
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -40,8 +40,8 @@ char *
>   fbsd_pid_to_exec_file (struct target_ops *self, int pid)
>   {
>     size_t len = PATH_MAX;
> -  char *buf = xcalloc (len, sizeof (char));
> -  char *path;
> +  static char buf[PATH_MAX];
> +  char *path, *ret;
>   
>   #ifdef KERN_PROC_PATHNAME
>     int mib[4];
> @@ -56,13 +56,12 @@ fbsd_pid_to_exec_file (struct target_ops
>   
>     path = xstrprintf ("/proc/%d/file", pid);
>     if (readlink (path, buf, PATH_MAX - 1) == -1)

readlink does not '\0' terminate, we need to do that ourselves.
See below.

> -    {
> -      xfree (buf);
> -      buf = NULL;
> -    }
> +    ret = NULL;
> +  else
> +    ret = buf;
>   
>     xfree (path);
> -  return buf;
> +  return ret;
>   }
>   
>   static int
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -4011,19 +4011,18 @@ linux_nat_thread_name (struct target_ops
>   static char *
>   linux_child_pid_to_exec_file (struct target_ops *self, int pid)
>   {
> -  char *name1, *name2;
> +  static char buf[PATH_MAX];
> +  char name1[PATH_MAX], name2[PATH_MAX];
>   
> -  name1 = xmalloc (PATH_MAX);
> -  name2 = xmalloc (PATH_MAX);
> -  make_cleanup (xfree, name1);
> -  make_cleanup (xfree, name2);
>     memset (name2, 0, PATH_MAX);
>   
>     xsnprintf (name1, PATH_MAX, "/proc/%d/exe", pid);
>     if (readlink (name1, name2, PATH_MAX - 1) > 0)
> -    return name2;
> +    strcpy (buf, name2);
>     else
> -    return name1;
> +    strcpy (buf, name1);
> +
> +  return buf;

No need for three buffers.  AFAICS, this should suffice:

  static char buf[PATH_MAX];
  char name[PATH_MAX];

  xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
  memset (buf, 0, PATH_MAX);
  if (readlink (name, buf, PATH_MAX - 1) <= 0)
    strcpy (buf, name);
  return buf;

>   }
>   
>   /* Records the thread's register state for the corefile note
> --- a/gdb/nbsd-nat.c
> +++ b/gdb/nbsd-nat.c
> @@ -28,16 +28,15 @@ char *
>   nbsd_pid_to_exec_file (struct target_ops *self, int pid)
>   {
>     size_t len = PATH_MAX;
> -  char *buf = xcalloc (len, sizeof (char));
> -  char *path;
> +  static char buf[PATH_MAX];
> +  char *path, *ret;
>   
>     path = xstrprintf ("/proc/%d/exe", pid);
>     if (readlink (path, buf, PATH_MAX - 1) == -1)
> -    {
> -      xfree (buf);
> -      buf = NULL;
> -    }
> +    ret = NULL;
> +  else
> +    ret = buf;
>   
>     xfree (path);
> -  return buf;
> +  return ret;
>   }

Same with the nul termination.  The most standard solution is:

  static char buf[PATH_MAX];
  char name[PATH_MAX];
  ssize_t len;

  xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
  len = readlink (name, buf, PATH_MAX - 1);
  if (len != -1)
    {
      buf[len] = '\0';
      return buf;
    }
  return NULL;

-- 
Pedro Alves