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

Jan Kratochvil
2012-12-18T17:16:20Z

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