[review] Use strtok_r instead of strtok

Message ID gerrit.1572715637000.Ief6138965a24398e5fc064598cd8f2abd3b5047c@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Nov. 2, 2019, 5:27 p.m. UTC
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/484
......................................................................

Use strtok_r instead of strtok

Improves threadsafety. Guile and Python can create threads, and the patch
series at https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176
will make GDB create threads too.

gdb/ChangeLog:

2019-11-02  Christian Biesinger  <cbiesinger@google.com>

	* linux-tdep.c (linux_info_proc): Use strtok_r instead of strtok.
	* mi/mi-main.c (output_cores): Likewise.
	* nat/linux-osdata.c (linux_xfer_osdata_cpus): Likewise.
	(linux_xfer_osdata_modules): Likewise.
	* remote.c (register_remote_support_xml): Likewise.
	* sparc64-tdep.c (adi_is_addr_mapped): Likewise.
	* xml-syscall.c (syscall_create_syscall_desc): Likewise.

gdb/gdbserver/ChangeLog:

2019-11-02  Christian Biesinger  <cbiesinger@google.com>

	* linux-x86-low.c (x86_linux_process_qsupported): Use strtok_r
	instead of strtok.
	* server.c (handle_query): Likewise.
	(captured_main): Likewise.

Change-Id: Ief6138965a24398e5fc064598cd8f2abd3b5047c
---
M gdb/gdbserver/linux-x86-low.c
M gdb/gdbserver/server.c
M gdb/linux-tdep.c
M gdb/mi/mi-main.c
M gdb/nat/linux-osdata.c
M gdb/remote.c
M gdb/sparc64-tdep.c
M gdb/xml-syscall.c
8 files changed, 36 insertions(+), 27 deletions(-)
  

Comments

Simon Marchi (Code Review) Nov. 6, 2019, 12:46 a.m. UTC | #1
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/484
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

I double-checked that we're pulling the strtok_r module in gnulib, so portability isn't an issue.

This LGTM.  Thanks for doing this.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/484/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/484/1//COMMIT_MSG@11 
PS1, Line 11: 
 6 | 
 7 | Use strtok_r instead of strtok
 8 | 
 9 | Improves threadsafety. Guile and Python can create threads, and the patch
10 | series at https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176
11 > will make GDB create threads too.
12 | 
13 | gdb/ChangeLog:
14 | 
15 | 2019-11-02  Christian Biesinger  <cbiesinger@google.com>
16 | 

One would hope that using strtok in our code has no effect to Guile and Python, unless they're using strtok themselves, which I really hope they're not, since that would be obviously broken. E.g., if multiple python threads end up in strtok.  I'd bet they don't do that.

This is mainly useful to protect against multiple _gdb_ threads using strtok.
  
Simon Marchi (Code Review) Nov. 6, 2019, 8:03 p.m. UTC | #2
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/484
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/484/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/484/1//COMMIT_MSG@11 
PS1, Line 11: 
 6 | 
 7 | Use strtok_r instead of strtok
 8 | 
 9 | Improves threadsafety. Guile and Python can create threads, and the patch
10 | series at https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176
11 > will make GDB create threads too.
12 | 
13 | gdb/ChangeLog:
14 | 
15 | 2019-11-02  Christian Biesinger  <cbiesinger@google.com>
16 | 

> One would hope that using strtok in our code has no effect to Guile and Python, unless they're using […]

Yes, good point. Updated the commit message and pushed.
  

Patch

diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index cafff6b..54bd2a2 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -912,9 +912,11 @@ 
       if (startswith (feature, "xmlRegisters="))
 	{
 	  char *copy = xstrdup (feature + 13);
-	  char *p;
 
-	  for (p = strtok (copy, ","); p != NULL; p = strtok (NULL, ","))
+	  char *saveptr;
+	  for (char *p = strtok_r (copy, ",", &saveptr);
+	       p != NULL;
+	       p = strtok_r (NULL, ",", &saveptr))
 	    {
 	      if (strcmp (p, "i386") == 0)
 		{
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 59e8a55..c5f7176 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2268,9 +2268,10 @@ 
 
 	  /* Two passes, to avoid nested strtok calls in
 	     target_process_qsupported.  */
-	  for (p = strtok (p + 1, ";");
+	  char *saveptr;
+	  for (p = strtok_r (p + 1, ";", &saveptr);
 	       p != NULL;
-	       p = strtok (NULL, ";"))
+	       p = strtok_r (NULL, ";", &saveptr))
 	    {
 	      count++;
 	      qsupported = XRESIZEVEC (char *, qsupported, count);
@@ -3633,12 +3634,11 @@ 
 	}
       else if (startswith (*next_arg, "--disable-packet="))
 	{
-	  char *packets, *tok;
-
-	  packets = *next_arg += sizeof ("--disable-packet=") - 1;
-	  for (tok = strtok (packets, ",");
+	  char *packets = *next_arg += sizeof ("--disable-packet=") - 1;
+	  char *saveptr;
+	  for (char *tok = strtok_r (packets, ",", &saveptr);
 	       tok != NULL;
-	       tok = strtok (NULL, ","))
+	       tok = strtok_r (NULL, ",", &saveptr))
 	    {
 	      if (strcmp ("vCont", tok) == 0)
 		disable_packet_vCont = true;
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 567b01c..18cee91 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -839,9 +839,10 @@ 
 			   "      Size", "    Offset", "objfile");
 	    }
 
-	  for (line = strtok (map.get (), "\n");
+	  char *saveptr;
+	  for (line = strtok_r (map.get (), "\n", &saveptr);
 	       line;
-	       line = strtok (NULL, "\n"))
+	       line = strtok_r (NULL, "\n", &saveptr))
 	    {
 	      ULONGEST addr, endaddr, offset, inode;
 	      const char *permissions, *device, *mapping_filename;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 0e99fa3..c14897a 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -696,8 +696,9 @@ 
   ui_out_emit_list list_emitter (uiout, field_name);
   auto cores = make_unique_xstrdup (xcores);
   char *p = cores.get ();
+  char *saveptr;
 
-  for (p = strtok (p, ","); p;  p = strtok (NULL, ","))
+  for (p = strtok_r (p, ",", &saveptr); p;  p = strtok_r (NULL, ",", &saveptr))
     uiout->field_string (NULL, p);
 }
 
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index 67f9f3a..84357e2 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -566,11 +566,12 @@ 
 	      char *key, *value;
 	      int i = 0;
 
-	      key = strtok (buf, ":");
+	      char *saveptr;
+	      key = strtok_r (buf, ":", &saveptr);
 	      if (key == NULL)
 		continue;
 
-	      value = strtok (NULL, ":");
+	      value = strtok_r (NULL, ":", &saveptr);
 	      if (value == NULL)
 		continue;
 
@@ -1216,36 +1217,36 @@ 
 	{
 	  if (fgets (buf, sizeof (buf), fp.get ()))
 	    {
-	      char *name, *dependencies, *status, *tmp;
+	      char *name, *dependencies, *status, *tmp, *saveptr;
 	      unsigned int size;
 	      unsigned long long address;
 	      int uses;
 
-	      name = strtok (buf, " ");
+	      name = strtok_r (buf, " ", &saveptr);
 	      if (name == NULL)
 		continue;
 
-	      tmp = strtok (NULL, " ");
+	      tmp = strtok_r (NULL, " ", &saveptr);
 	      if (tmp == NULL)
 		continue;
 	      if (sscanf (tmp, "%u", &size) != 1)
 		continue;
 
-	      tmp = strtok (NULL, " ");
+	      tmp = strtok_r (NULL, " ", &saveptr);
 	      if (tmp == NULL)
 		continue;
 	      if (sscanf (tmp, "%d", &uses) != 1)
 		continue;
 
-	      dependencies = strtok (NULL, " ");
+	      dependencies = strtok_r (NULL, " ", &saveptr);
 	      if (dependencies == NULL)
 		continue;
 
-	      status = strtok (NULL, " ");
+	      status = strtok_r (NULL, " ", &saveptr);
 	      if (status == NULL)
 		continue;
 
-	      tmp = strtok (NULL, "\n");
+	      tmp = strtok_r (NULL, "\n", &saveptr);
 	      if (tmp == NULL)
 		continue;
 	      if (sscanf (tmp, "%llx", &address) != 1)
diff --git a/gdb/remote.c b/gdb/remote.c
index 8ea52d3..1ac9013 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5169,7 +5169,8 @@ 
   else
     {
       char *copy = xstrdup (remote_support_xml + 13);
-      char *p = strtok (copy, ",");
+      char *saveptr;
+      char *p = strtok_r (copy, ",", &saveptr);
 
       do
 	{
@@ -5180,7 +5181,7 @@ 
 	      return;
 	    }
 	}
-      while ((p = strtok (NULL, ",")) != NULL);
+      while ((p = strtok_r (NULL, ",", &saveptr)) != NULL);
       xfree (copy);
 
       remote_support_xml = reconcat (remote_support_xml,
diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
index 873fbaa..fc73b23 100644
--- a/gdb/sparc64-tdep.c
+++ b/gdb/sparc64-tdep.c
@@ -316,8 +316,10 @@ 
   if (data)
     {
       adi_stat_t adi_stat = get_adi_info (pid);
-      char *line;
-      for (line = strtok (data.get (), "\n"); line; line = strtok (NULL, "\n"))
+      char *saveptr;
+      for (char *line = strtok_r (data.get (), "\n", &saveptr);
+	   line;
+	   line = strtok_r (NULL, "\n", &saveptr))
         {
           ULONGEST addr, endaddr;
 
diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
index dc988df..3830faa 100644
--- a/gdb/xml-syscall.c
+++ b/gdb/xml-syscall.c
@@ -221,9 +221,10 @@ 
   /*  Add syscall to its groups.  */
   if (groups != NULL)
     {
-      for (char *group = strtok (groups, ",");
+      char *saveptr;
+      for (char *group = strtok_r (groups, ",", &saveptr);
 	   group != NULL;
-	   group = strtok (NULL, ","))
+	   group = strtok_r (NULL, ",", &saveptr))
 	syscall_group_add_syscall (syscalls_info, sysdesc, group);
     }
 }