Re: [PATCH/RFC 02/02 v2] Refactor PRPSINFO handling on GDB

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

On Mon, 17 Dec 2012 04:09:57 +0100, Sergio Durigan Junior wrote:
> I have tested this code on x86_64 (with -m32 too), PPC64 (with -m32 too)
> and ARM.  I read the corefile generated using eu-readelf and it
> correctly displayed the PRPSINFO section.

There could be a testcase using eu-readelf but I do not insist on it.


> 2012-12-17  Sergio Durigan Junior  <sergiodj@redhat.com>

AFAIK it is based on an older patch by Denys Vlasenko.


[...]
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c

linux-nat.c (and therefore neither linux-nat.h) should no longer be touched,
it should be in linux-tdep.c now, core generating code has moved in the
meantime.


> @@ -67,6 +67,8 @@
>  #include "linux-ptrace.h"
>  #include "buffer.h"
>  #include "target-descriptions.h"
> +#include "cli/cli-utils.h"
> +#include "elf-psinfo.h"		/* for `struct elf_internal_prpsinfo' */
>  
>  #ifndef SPUFS_MAGIC
>  #define SPUFS_MAGIC 0x23c9b64e
> @@ -4368,6 +4370,168 @@ linux_nat_collect_thread_registers (const struct regcache *regcache,
>    return note_data;
>  }
>  
> +/* See definition at linux-nat.h.  */
> +
> +int
> +linux_nat_fill_prpsinfo (struct elf_internal_prpsinfo *p)
> +{
> +  /* The filename which we will use to obtain some info about the process.
> +     We will basically use this to store the `/proc/PID/FILENAME' file.  */
> +  char filename[100];
> +  /* The full name of the program which generated the corefile.  */
> +  char *fname;
> +  /* The basename of the executable.  */
> +  const char *basename;
> +  /* The arguments of the program.  */
> +  char *psargs;
> +  char *infargs;
> +  /* The contents of `/proc/PID/stat' file.  */
> +  char *proc_stat;
> +  /* The valid states of a process, according to the Linux kernel.  */
> +  const char valid_states[] = "RSDTZW";
> +  /* The state of the process.  */
> +  char pr_sname;
> +  /* The PID of the program which generated the corefile.  */
> +  pid_t pid;
> +  /* Parent PID, process group ID and session ID.  */
> +  int pr_ppid, pr_pgrp, pr_sid;
> +  /* Process flags.  */
> +  unsigned int pr_flag;
> +  /* Process nice value.  */
> +  long pr_nice;
> +  /* The stat of the `/proc/PID/stat' file.  */
> +  struct stat proc_st;
> +  /* The number of fields read by `sscanf'.  */
> +  int n_fields = 0;
> +  /* Cleanups.  */
> +  struct cleanup *c;
> +  int i;
> +
> +  gdb_assert (p != NULL);
> +
> +  /* Initializing the cleanup chain.  */
> +  c = make_cleanup (null_cleanup, NULL);

One could initialize C by the first real make_cleanup below.


> +
> +  /* Obtaining PID and filename.  */
> +  pid = ptid_get_pid (inferior_ptid);
> +  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", pid);
> +  fname = target_fileio_read_stralloc (filename);
> +
> +  if (fname == NULL || strcmp (fname, "") == 0)

Why strcmp, GDB uses just *fname == '\0'.


> +    {
> +      /* No program name was read, so we won't be able to retrieve more
> +	 information about the process.  */

FNAME leaks if it was "".


C is not destroyed.


> +      return 0;
> +    }
> +
> +  make_cleanup (xfree, fname);
> +  memset (p, 0, sizeof (*p));
> +
> +  /* Obtaining the file stat as well.  */
> +  stat (filename, &proc_st);

One could print a warning on failed stat.  And if it fails pr_uid and pr_gid
could have better default than the uninitialized data.


> +
> +  /* Defining the PID.  */
> +  p->pr_pid = pid;
> +
> +  /* Copying the program name.  Only the basename matters.  */
> +  basename = lbasename (fname);
> +  strncpy (p->pr_fname, basename, sizeof (p->pr_fname));
> +  p->pr_fname[sizeof (p->pr_fname) - 1] = '\0';
> +
> +  /* Generating and copying the program's arguments.  */
> +  psargs = xstrdup (fname);
> +  infargs = get_inferior_args ();

get_inferior_args can throw, PSARGS leaks then.


> +
> +  if (infargs != NULL)
> +    psargs = reconcat (psargs, psargs, " ", infargs, NULL);
> +
> +  make_cleanup (xfree, psargs);
> +
> +  strncpy (p->pr_psargs, psargs, sizeof (p->pr_psargs));
> +  p->pr_psargs[sizeof (p->pr_psargs) - 1] = '\0';
> +
> +  xsnprintf (filename, sizeof (filename), "/proc/%d/stat", pid);

pid is pid_t, I believe on some systems it may be incompatible with %d.


> +  proc_stat = target_fileio_read_stralloc (filename);

PROC_STAT leaks.


> +
> +  if (proc_stat == NULL || strcmp (proc_stat, "") == 0)

Again *proc_stat == '\0'.


> +    {
> +      /* Despite being unable to read more information about the process, we
> +	 return 1 here because at least we have its command line, PID and
> +	 arguments.  */
> +      do_cleanups (c);
> +      return 1;
> +    }
> +
> +  /* Ok, we have the stats.  It's time to do a little parsing of the contents
> +     of the buffer, so that we end up reading what we want.
> +
> +     The following parsing mechanism is strongly based on the information
> +     generated by the `fs/proc/array.c' file, present in the Linux kernel
> +     tree.  More details about how the information is displayed can be
> +     obtained by seeing the manpage of proc(5), specifically under the entry
> +     of `/proc/[pid]/stat'.  */
> +
> +  /* Getting rid of the PID, since we already have it.  */
> +  while (isdigit (*proc_stat))
> +    ++proc_stat;
> +
> +  proc_stat = skip_spaces (proc_stat);
> +
> +  /* Getting rid of the executable name, since we already have it.  We know
> +     that this name will be in parentheses, so we can safely look for the
> +     close-paren.  */
> +  while (*proc_stat != ')')
> +    ++proc_stat;
> +  ++proc_stat;
> +
> +  proc_stat = skip_spaces (proc_stat);
> +
> +  n_fields = sscanf (proc_stat,
> +		     "%c " /* Process state.  */
> +		     "%d %d %d " /* Parent PID, group ID, session ID.  */
> +		     "%*d %*d " /* tty_nr, tpgid (not used).  */
> +		     "%u " /* Flags.  */
> +		     "%*s %*s %*s %*s " /* minflt, cminflt, majflt,
> +					       cmajflt (not used).  */
> +		     "%*s %*s %*s %*s " /* utime, stime, cutime,
> +					       cstime (not used).  */
> +		     "%*s " /* Priority (not used).  */
> +		     "%ld ", /* Nice.  */

The comments could be better tab aligned into some column.  Also spaces in the
sscanf string are not needed.


> +		     &pr_sname,
> +		     &pr_ppid, &pr_pgrp, &pr_sid,
> +		     &pr_flag,
> +		     &pr_nice);

I think you could read some of the fields - at least pr_ppid - directly into P
without local variables.


> +
> +  if (n_fields != 6)
> +    {
> +      /* Again, we couldn't read the complementary information about the
> +	 process state.  However, we already have minimal information, so we
> +	 just return 1 here.  */
> +      do_cleanups (c);
> +      return 1;
> +    }
> +
> +  /* Filling the structure fields.  */
> +  for (i = 0; i < sizeof (valid_states); ++i)
> +    if (pr_sname == valid_states[i])
> +      break;

Do you find it with strchr more complicated?  Also it does not sanity check
the read-in PR_SNAME value.


> +
> +  p->pr_state = i;
> +  p->pr_sname = pr_sname;
> +  p->pr_zomb = pr_sname == 'Z';
> +  p->pr_nice = pr_nice;
> +  p->pr_flag = pr_flag;
> +  p->pr_uid = proc_st.st_uid;
> +  p->pr_gid = proc_st.st_gid;
> +  p->pr_ppid = pr_ppid;
> +  p->pr_pgrp = pr_pgrp;
> +  p->pr_sid = pr_sid;
> +
> +  do_cleanups (c);
> +
> +  return 1;
> +}

Otherwise I am fine with it after patch 1/2 gets resolved.


Thanks,
Jan