[patch] Fix crash on process name "(sd-pam)" (PR 16594)

[patch] Fix crash on process name "(sd-pam)" (PR 16594)

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]

Hi,

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.


Thanks,
Jan

[-- Attachment #2: info-os-processes.patch --]
[-- Type: text/plain, Size: 2062 bytes --]

gdb/
2014-02-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	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.

diff --git a/gdb/common/linux-osdata.c b/gdb/common/linux-osdata.c
index 805850c..37d78a0 100644
--- a/gdb/common/linux-osdata.c
+++ b/gdb/common/linux-osdata.c
@@ -98,10 +98,10 @@ linux_common_core_of_thread (ptid_t ptid)
 
   p = strchr (content, '(');
 
-  /* Skip ")".  */
+  /* Skip ")".  Handle also process names like "((sd-pam))".  */
   if (p != NULL)
     p = strchr (p, ')');
-  if (p != NULL)
+  while (p != NULL && *p != 0 && *p != ' ')
     p++;
 
   /* If the first field after program name has index 0, then core number is
@@ -258,11 +258,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 +285,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 +349,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)

[patchv2] Fix crash on process name "(sd-pam)" (PR 16594)

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

Hi,

I have added two other cases not handling it well.

It can IMO never work perfectly, the file format is ambiguous.
Or maybe one could parse it in opposite direction, from the end.

------------------------------------------------------------------------------
(gdb) info proc stat
process 6680
Process: 6680
Exec file: (sd-pam
State: )
Parent process: 0
Process group: 0
Session id: 0
TTY: 0
TTY owner process group: 0
Flags: 0x0
Minor faults (no memory page): 0
Minor faults, children: 0
Major faults (memory page faults): 0
Major faults, children: 0
utime: 0
stime: 0
utime, children: 0
stime, children: 0
jiffies remaining in current time slice: 0
'nice' value: 0
jiffies until next timeout: 0
jiffies until next SIGALRM: 0
start time (jiffies since system boot): 0
Virtual memory size: 0
Resident set size: 0
rlim: 0
Start of text: 0x0
End of text: 0x0
Start of stack: 0x0

->


(gdb) info proc stat
process 6680
Process: 6680
Exec file: (sd-pam)
State: t
Parent process: 6678
Process group: 6678
Session id: 6678
TTY: 0
TTY owner process group: 18446744073709551615
Flags: 0x40406140
Minor faults (no memory page): 46
Minor faults, children: 0
Major faults (memory page faults): 7
Major faults, children: 0
utime: 0
stime: 0
utime, children: 0
stime, children: 0
jiffies remaining in current time slice: 20
'nice' value: 0
jiffies until next timeout: 1
jiffies until next SIGALRM: 0
start time (jiffies since system boot): 18568
Virtual memory size: 73768960
Resident set size: 554
rlim: 18446744073709551615
Start of text: 0x7f0294f7d000
End of text: 0x7f0295085ba3
Start of stack: 0x7fff3e302b30
------------------------------------------------------------------------------
+
------------------------------------------------------------------------------
(gdb) gcore ...
debug dump: n_fields=1 pr_sname=) ppid=0
->
debug dump: n_fields=6 pr_sname=t ppid=6678
Saved corefile ...
------------------------------------------------------------------------------

Maybe it would be worth a testcase.


Thanks,
Jan

[-- Attachment #2: info-os-processes.patch --]
[-- Type: text/plain, Size: 3197 bytes --]

gdb/
2014-02-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	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.

diff --git a/gdb/common/linux-osdata.c b/gdb/common/linux-osdata.c
index 805850c..37d78a0 100644
--- a/gdb/common/linux-osdata.c
+++ b/gdb/common/linux-osdata.c
@@ -98,10 +98,10 @@ linux_common_core_of_thread (ptid_t ptid)
 
   p = strchr (content, '(');
 
-  /* Skip ")".  */
+  /* Skip ")".  Handle also process names like "((sd-pam))".  */
   if (p != NULL)
     p = strchr (p, ')');
-  if (p != NULL)
+  while (p != NULL && *p != 0 && *p != ' ')
     p++;
 
   /* If the first field after program name has index 0, then core number is
@@ -258,11 +258,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 +285,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 +349,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 bd1e5a2..24229bf 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -479,9 +479,12 @@ linux_info_proc (struct gdbarch *gdbarch, char *args,
 	      const char *ep = strchr (p, ')');
 	      if (ep != NULL)
 		{
+		  /* Handle also process names like "((sd-pam))".  */
+		  while (*ep != 0 && *ep != ' ')
+		    ep++;
 		  printf_filtered ("Exec file: %.*s\n",
-				   (int) (ep - p - 1), p + 1);
-		  p = ep + 1;
+				   (int) (ep - p - 2), p + 1);
+		  p = ep;
 		}
 	    }
 
@@ -1333,10 +1336,11 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
 
   /* 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.  */
+     for the close-paren.  Handle also process names like "((sd-pam))".  */
   while (*proc_stat != ')')
     ++proc_stat;
-  ++proc_stat;
+  while (*proc_stat != 0 && *proc_stat != ' ')
+    ++proc_stat;
 
   proc_stat = skip_spaces (proc_stat);
 

Re: [patchv2] Fix crash on process name "(sd-pam)" (PR 16594)

[Pedro Alves]

On 02/17/2014 10:04 PM, Jan Kratochvil wrote:
> Hi,
> 
> I have added two other cases not handling it well.
> 
> It can IMO never work perfectly, the file format is ambiguous.
> Or maybe one could parse it in opposite direction, from the end.

Another idea that crossed my mind would be to extract the exec
name from /proc//status or /proc//comm first, and then skip
that string when parsing /proc//stat.  E.g.:

$ cp /bin/cat 'cat 2 ) '
$ ./'cat 2 ) ' /proc/self/status
Name:   cat 2 )
...

$ ./'cat 2 ) ' /proc/self/stat
22556 (cat 2 ) ) R 1525 22556...

But of course that be racy (not that the current code isn't, btw...)

But, thinking again about the "from the end" idea.
I'm under the impression that new fields have been
appended to stat over the years.  But that doesn't seem
to matter, I think there's an unambiguous way to parse this.
We don't actually need to start at the very end of the whole
stat line.  This might be simpler than first looks actually.
See:

 $ cp /bin/cat 1234567890abcdef
 $ ./1234567890abcdef /proc/self/stat
 22804 (1234567890abcde) R 1525 22804 1525 34819 22804 4218880 197 0 0 0 0 0 0 0 20 0 1 0 26563756 109436928 126 18446744073709551615 4194304 4238049 140734145129136 140734145129136 215776905456 0 0 0 0 0 0 0 17 0 0 0 0 0 0 6335480 6337168 31199232 140734145134299 140734145134334 140734145134334 140734145138661 0

Notice that comm/name field is trimmed to 15 chars.  I'd guess 15 to
be related to TASK_COMM_LEN, maybe that minus 2 for the parens.
Haven't looked at the sources.

Given that even if the task name is empty (if even possible),
it's guaranteed that in 17 chars after PID we won't see a
')' that is _not_ part of the name field, we just need to
read 17 chars, and search from the end of that for the first ')'.

-- 
Pedro Alves

Re: [patchv2] Fix crash on process name "(sd-pam)" (PR 16594)

[Pedro Alves]

On 02/21/2014 11:30 AM, Pedro Alves wrote:

> But, thinking again about the "from the end" idea.
> I'm under the impression that new fields have been
> appended to stat over the years.  But that doesn't seem
> to matter, I think there's an unambiguous way to parse this.
> We don't actually need to start at the very end of the whole
> stat line.  This might be simpler than first looks actually.
> See:
> 
>  $ cp /bin/cat 1234567890abcdef
>  $ ./1234567890abcdef /proc/self/stat
>  22804 (1234567890abcde) R 1525 22804 1525 34819 22804 4218880 197 0 0 0 0 0 0 0 20 0 1 0 26563756 109436928 126 18446744073709551615 4194304 4238049 140734145129136 140734145129136 215776905456 0 0 0 0 0 0 0 17 0 0 0 0 0 0 6335480 6337168 31199232 140734145134299 140734145134334 140734145134334 140734145138661 0
> 
> Notice that comm/name field is trimmed to 15 chars.  I'd guess 15 to
> be related to TASK_COMM_LEN, maybe that minus 2 for the parens.
> Haven't looked at the sources.
> 
> Given that even if the task name is empty (if even possible),
> it's guaranteed that in 17 chars after PID we won't see a
> ')' that is _not_ part of the name field, we just need to
> read 17 chars, and search from the end of that for the first ')'.

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.

http://procps.cvs.sourceforge.net/viewvc/procps/procps/proc/readproc.c?revision=1.57&view=markup

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.

-- 
Pedro Alves

Re: [patchv2] Fix crash on process name "(sd-pam)" (PR 16594)

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On Fri, 21 Feb 2014 12:55:45 +0100, Pedro Alves wrote:
> 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.

Done.


> http://procps.cvs.sourceforge.net/viewvc/procps/procps/proc/readproc.c?revision=1.57&view=markup

The current procps-ng source is (the stat2proc() code is the same there):

https://gitorious.org/procps/procps/source/dc072aced7250fed9b01fb05f0d672678752a63e:proc/readproc.c


Thanks,
Jan

[-- Attachment #2: info-os-processes.patch --]
[-- Type: text/plain, Size: 3192 bytes --]

gdb/
2014-02-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	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.

diff --git a/gdb/common/linux-osdata.c b/gdb/common/linux-osdata.c
index 805850c..dae637b 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 bd1e5a2..c10b8ee 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);
 

Re: [patchv2] Fix crash on process name "(sd-pam)" (PR 16594)

[Pedro Alves]

On 02/21/2014 05:24 PM, Jan Kratochvil wrote:
> On Fri, 21 Feb 2014 12:55:45 +0100, Pedro Alves wrote:
>> 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.
> 
> Done.

Looks good.  Thanks.

-- 
Pedro Alves

[commit] [patchv2] Fix crash on process name "(sd-pam)" (PR 16594)

[Jan Kratochvil]

On Fri, 21 Feb 2014 18:35:44 +0100, Pedro Alves wrote:
> Looks good.  Thanks.

Checked in:
	184cd07257b5dd74a4eb9f6857fc6dd785f53492


Jan