[review] Use strtok_r instead of strtok

Message ID gerrit.1572715637000.Ief6138965a24398e5fc064598cd8f2abd3b5047c@gnutoolchain-gerrit.osci.io
State New
Headers show
Series
  • [review] Use strtok_r instead of strtok
Related show

Commit Message

Simon Marchi (Code Review) Nov. 2, 2019, 5:27 p.m.
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(-)




-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ief6138965a24398e5fc064598cd8f2abd3b5047c
Gerrit-Change-Number: 484
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-MessageType: newchange

Comments

Simon Marchi (Code Review) Nov. 6, 2019, 12:46 a.m. | #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.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ief6138965a24398e5fc064598cd8f2abd3b5047c
Gerrit-Change-Number: 484
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 00:46:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 6, 2019, 8:03 p.m. | #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.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ief6138965a24398e5fc064598cd8f2abd3b5047c
Gerrit-Change-Number: 484
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 20:03:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment

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);
     }
 }