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

[Mark Wielaard <mjw at redhat dot com>]

On Wed, Dec 25, 2013 at 08:23:25PM +0100, Jan Kratochvil wrote:
> 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'.

Yeah, changed to use getline, just in case.
We should avoid fgets in general, only getline really is safe.

> > +      {
> > +        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.

It would be zero in the very unlikely case atoi doesn't see any digits.

> Otherwise OK with me.

Thanks, pushed.

Minus the one line src/stack change, which I moved into the
stack: Simplify argument parsing commit on mjw/pending.

Mark