Re: [PATCH] libdwfl: linux-pid-attach.c: Report actual PID (Tgid) to dwfl_attach_state.

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Mon, 23 Dec 2013 14:22:24 +0100, Mark Wielaard wrote:
[...]
> --- a/libdwfl/linux-pid-attach.c
> +++ b/libdwfl/linux-pid-attach.c
> @@ -301,6 +301,27 @@ bool
>  internal_function
>  __libdwfl_attach_state_for_pid (Dwfl *dwfl, pid_t pid)
>  {
> +  char buffer[36];
> +  FILE *procfile;
> +
> +  /* Make sure to report the actual PID (thread group leader) to
> +     dwfl_attach_state.  */
> +  snprintf (buffer, sizeof (buffer), "/proc/%ld/status", (long) pid);
> +  procfile = fopen (buffer, "r");
> +  if (procfile == NULL)
> +    return false;
> +
> +  while (fgets (buffer, sizeof (buffer), procfile) != NULL)
> +    if (strncmp (buffer, "Tgid:", 5) == 0)

It works in practice but I do not find the code too much safe.

'buffer' is too small, /proc/*/status have lines longer than 36 chars,
therefore strncmp will be applied in middle of lines.  Fortunately Tgid: is
present before the longer lines (but will it always be so?).  The first
'Name:' line is max. 22 bytes incl. '\0'.



> +      {
> +        pid = atoi (&buffer[5]);
> +        break;
> +      }
> +  fclose (procfile);
> +
> +  if (pid == 0)
> +    return false;

I do not understand this conditional.  If "Tgid:" was not found PID will be
the user-specified TID, not 0.


> +
>    char dirname[64];
>    int i = snprintf (dirname, sizeof (dirname), "/proc/%ld/task", (long) pid);
>    assert (i > 0 && i < (ssize_t) sizeof (dirname) - 1);
[...]


Otherwise OK with me.


Thanks,
Jan