From 184cd07257b5dd74a4eb9f6857fc6dd785f53492 Mon Sep 17 00:00:00 2001 From: Jan Kratochvil Date: Fri, 21 Feb 2014 18:39:40 +0100 Subject: [PATCH] Fix crash on process name "(sd-pam)" (PR 16594). info os processes -fsanitize=address error https://sourceware.org/bugzilla/show_bug.cgi?id=16594 info os processes ================================================================= ==5795== ERROR: AddressSanitizer: heap-use-after-free on address 0x600600214974 at pc 0x757a92 bp 0x7fff95dd9f00 sp 0x7fff95dd9ef0 READ of size 4 at 0x600600214974 thread T0 #0 0x757a91 in get_cores_used_by_process (.../gdb/gdb+0x757a91) At least Fedora 20 has process(es): 6678 ? Ss 0:00 /usr/lib/systemd/systemd --user 6680 ? S 0:00 \_ (sd-pam) and GDB "info os processes" crashes on it as /proc/6680/stat contains: 6680 ((sd-pam)) S 6678 6678 6678 0 -1 1077961024 33 0 0 0 0 0 0 0 20 0 1 0 18568 73768960 120 18446744073709551615 1 1 0 0 0 0 0 4096 0 18446744073709551615 0 0 17 6 0 0 0 0 0 0 0 0 0 0 0 0 0 and GDB fails to find the proper end of the process name "((sd-pam))". Therefore it reads core number off-by-one (it reads 17 instead of 6) and overruns the array. (1) Make the process name parsing more foolproof. (2) Do not trust the parsed number from /proc/PID/stat and verify it against the array size. I noticed that 'ps' gets this right, so I've peeked at its sources, and it just looks for the first ')' starting at the end. https://gitorious.org/procps/procps/source/dc072aced7250fed9b01fb05f0d672678752a63e:proc/readproc.c Look for stat2proc. Given ps does that, I believe the kernel won't ever be changed in a way that would break it. So it sounds like could do strrchr from the end of stat just as well without worry, which is simpler. gdb/ 2014-02-21 Jan Kratochvil PR gdb/16594 * common/linux-osdata.c (linux_common_core_of_thread): Find the end of process name. (get_cores_used_by_process): New parameter num_cores, use it. (linux_xfer_osdata_processes): Pass num_cores to it. * linux-tdep.c (linux_info_proc, linux_fill_prpsinfo): Find the end of process name. Message-ID: <20140217212826.GA15080@host2.jankratochvil.net> --- gdb/ChangeLog | 10 ++++++++++ gdb/common/linux-osdata.c | 16 ++++++---------- gdb/linux-tdep.c | 18 +++++++++++------- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 9406d76922a..f97c5fe7dbe 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,13 @@ +2014-02-21 Jan Kratochvil + + PR gdb/16594 + * common/linux-osdata.c (linux_common_core_of_thread): Find the end of + process name. + (get_cores_used_by_process): New parameter num_cores, use it. + (linux_xfer_osdata_processes): Pass num_cores to it. + * linux-tdep.c (linux_info_proc, linux_fill_prpsinfo): Find the end of + process name. + 2014-02-21 Andreas Arnez * target.c (memory_xfer_partial): Fix length arg in call to diff --git a/gdb/common/linux-osdata.c b/gdb/common/linux-osdata.c index 805850cbbc9..dae637bd930 100644 --- a/gdb/common/linux-osdata.c +++ b/gdb/common/linux-osdata.c @@ -96,11 +96,8 @@ linux_common_core_of_thread (ptid_t ptid) } } - p = strchr (content, '('); - - /* Skip ")". */ - if (p != NULL) - p = strchr (p, ')'); + /* ps command also relies on no trailing fields ever contain ')'. */ + p = strrchr (content, ')'); if (p != NULL) p++; @@ -258,11 +255,10 @@ get_process_owner (uid_t *owner, PID_T pid) } /* Find the CPU cores used by process PID and return them in CORES. - CORES points to an array of at least sysconf(_SC_NPROCESSOR_ONLN) - elements. */ + CORES points to an array of NUM_CORES elements. */ static int -get_cores_used_by_process (PID_T pid, int *cores) +get_cores_used_by_process (PID_T pid, int *cores, const int num_cores) { char taskdir[sizeof ("/proc/") + MAX_PID_T_STRLEN + sizeof ("/task") - 1]; DIR *dir; @@ -286,7 +282,7 @@ get_cores_used_by_process (PID_T pid, int *cores) core = linux_common_core_of_thread (ptid_build ((pid_t) pid, (pid_t) tid, 0)); - if (core >= 0) + if (core >= 0 && core < num_cores) { ++cores[core]; ++task_count; @@ -350,7 +346,7 @@ linux_xfer_osdata_processes (gdb_byte *readbuf, /* Find CPU cores used by the process. */ cores = (int *) xcalloc (num_cores, sizeof (int)); - task_count = get_cores_used_by_process (pid, cores); + task_count = get_cores_used_by_process (pid, cores, num_cores); cores_str = (char *) xcalloc (task_count, sizeof ("4294967295") + 1); for (i = 0; i < num_cores && task_count > 0; ++i) diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index bd1e5a2f36c..c10b8eed54c 100644 --- a/gdb/linux-tdep.c +++ b/gdb/linux-tdep.c @@ -476,7 +476,9 @@ linux_info_proc (struct gdbarch *gdbarch, char *args, p = skip_spaces_const (p); if (*p == '(') { - const char *ep = strchr (p, ')'); + /* ps command also relies on no trailing fields + ever contain ')'. */ + const char *ep = strrchr (p, ')'); if (ep != NULL) { printf_filtered ("Exec file: %.*s\n", @@ -1331,12 +1333,14 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p) 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; + /* ps command also relies on no trailing fields ever contain ')'. */ + proc_stat = strrchr (proc_stat, ')'); + if (proc_stat == NULL) + { + do_cleanups (c); + return 1; + } + proc_stat++; proc_stat = skip_spaces (proc_stat); -- 2.30.2