Re: [patch] Fix crash on /proc/PID/stat race

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Fri, 28 May 2010 00:25:40 +0200, Pedro Alves wrote:
> Yes, exactly.  Thanks.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-05/msg00243.html


> As long as a process hasn't been waited for, the /stat entry should exist,
> even if the process is zombie.

I agree but still finding such external dependency needlessly fragile.


> Anyway, if you want to put that one in, it's okay, but please don't
> lose the comment below:
> 
> > -  p = strchr (p, ')') + 2; /* skip ")" and a whitespace. */
> > +  if (p != NULL)
> > +    p = strchr (p, ')');

The part "and a whitespace." was removed as redundant (due to the following
strtok_r) while getting more complicated making it safe.  In the spirit of GNU
Coding Style
	"It is not necessary to duplicate in words the meaning of the
	C argument declarations"
(while used in a different meaning at that point) I removed the remainder of
the comment
	/* skip ")" */
for the block
	  if (p != NULL)
	    p = strchr (p, ')');
	  if (p != NULL)
	    p++;
as obvious; but I put it back now on your request.


> and I don't think the fopen race comment makes sense > as is anymore.

OK, removed that comment.


> Also, IWBN if gdbserver was fixed similarly, but I won't ask
> you to do that.  :-)

Forgot/unaware, fixed the same way.

Checked-in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-05/msg00244.html

--- src/gdb/ChangeLog	2010/05/28 18:00:41	1.11855
+++ src/gdb/ChangeLog	2010/05/28 18:23:13	1.11856
@@ -1,5 +1,10 @@
 2010-05-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	* linux-nat.c (linux_nat_core_of_thread_1): Fix crash on invalid
+	CONTENT.
+
+2010-05-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	* linux-nat.c (linux_nat_wait_1): Do not call
 	linux_nat_core_of_thread_1 on TARGET_WAITKIND_EXITED or
 	TARGET_WAITKIND_SIGNALLED.
--- src/gdb/gdbserver/ChangeLog	2010/05/26 22:40:22	1.386
+++ src/gdb/gdbserver/ChangeLog	2010/05/28 18:23:15	1.387
@@ -1,3 +1,8 @@
+2010-05-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* linux-low.c (linux_core_of_thread): Fix crash on invalid CONTENT.
+	New comment.
+
 2010-05-26  Ozkan Sezer  <sezeroz@gmail.com>
 
 	* gdbreplay.c (remote_open): Check error return from socket() call by
--- src/gdb/linux-nat.c	2010/05/28 18:00:46	1.169
+++ src/gdb/linux-nat.c	2010/05/28 18:23:15	1.170
@@ -5509,15 +5509,21 @@
   make_cleanup (xfree, content);
 
   p = strchr (content, '(');
-  p = strchr (p, ')') + 2; /* skip ")" and a whitespace. */
+
+  /* Skip ")".  */
+  if (p != NULL)
+    p = strchr (p, ')');
+  if (p != NULL)
+    p++;
 
   /* If the first field after program name has index 0, then core number is
      the field with index 36.  There's no constant for that anywhere.  */
-  p = strtok_r (p, " ", &ts);
-  for (i = 0; i != 36; ++i)
+  if (p != NULL)
+    p = strtok_r (p, " ", &ts);
+  for (i = 0; p != NULL && i != 36; ++i)
     p = strtok_r (NULL, " ", &ts);
 
-  if (sscanf (p, "%d", &core) == 0)
+  if (p == NULL || sscanf (p, "%d", &core) == 0)
     core = -1;
 
   do_cleanups (back_to);
--- src/gdb/gdbserver/linux-low.c	2010/05/03 04:02:20	1.148
+++ src/gdb/gdbserver/linux-low.c	2010/05/28 18:23:15	1.149
@@ -4346,13 +4346,21 @@
     }
 
   p = strchr (content, '(');
-  p = strchr (p, ')') + 2; /* skip ")" and a whitespace. */
 
-  p = strtok_r (p, " ", &ts);
-  for (i = 0; i != 36; ++i)
+  /* Skip ")".  */
+  if (p != NULL)
+    p = strchr (p, ')');
+  if (p != NULL)
+    p++;
+
+  /* If the first field after program name has index 0, then core number is
+     the field with index 36.  There's no constant for that anywhere.  */
+  if (p != NULL)
+    p = strtok_r (p, " ", &ts);
+  for (i = 0; p != NULL && i != 36; ++i)
     p = strtok_r (NULL, " ", &ts);
 
-  if (sscanf (p, "%d", &core) == 0)
+  if (p == NULL || sscanf (p, "%d", &core) == 0)
     core = -1;
 
   free (content);