[binutils-gdb] Make parse_static_tracepoint_marker_definition work with multiple static tracepoint definitions

[Simon Marchi <simark at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=62c222b6d9fcce8adf65f48fca2e528f777afeeb

commit 62c222b6d9fcce8adf65f48fca2e528f777afeeb
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Thu Mar 22 00:26:37 2018 -0400

    Make parse_static_tracepoint_marker_definition work with multiple static tracepoint definitions
    
    Since I modify the parse_static_tracepoint_marker_definition function in
    the next patch, I wanted to write a unit test for it.  Doing so showed
    that it doesn't handle multiple consecutive static tracepoint
    definitions separated by commas.  However, the RSP documentation [1]
    states that servers may return multiple definitions, like:
    
      1234:6d61726b657231:6578747261207374756666,abba:6d61726b657232:
    
    The problem is that the function uses strlen to compute the length of
    the last field (the extra field).  If there are additional definitions
    in addition to the one we are currently parsing, the returned length
    will include those definitions, and we'll try to hex-decode past the
    extra field.
    
    This patch changes parse_static_tracepoint_marker_definition to consider
    the case where the current definition is followed by a comma and more
    definitions.  It also adds the unit test that found the issue in the
    first place.
    
    I don't think this causes any backwards compatibility issues, because
    the previous code only handled single static tracepoint definitions, and
    the new code handles that correctly.
    
    gdb/ChangeLog:
    
    	* tracepoint.c (parse_static_tracepoint_marker_definition):
    	Consider case where the definition is followed by more
    	definitions.
    	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
    	tracepoint-selftests.c.
    	* unittests/tracepoint-selftests.c: New.
    
    [1] https://sourceware.org/gdb/onlinedocs/gdb/Tracepoint-Packets.html#qTfSTM

Diff:
---
 gdb/ChangeLog                        |  9 +++++
 gdb/Makefile.in                      |  1 +
 gdb/tracepoint.c                     | 14 ++++++--
 gdb/unittests/tracepoint-selftests.c | 70 ++++++++++++++++++++++++++++++++++++
 4 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1ed8824..6af571c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2018-03-22  Simon Marchi  <simon.marchi@polymtl.ca>
+
+	* tracepoint.c (parse_static_tracepoint_marker_definition):
+	Consider case where the definition is followed by more
+	definitions.
+	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
+	tracepoint-selftests.c.
+	* unittests/tracepoint-selftests.c: New.
+
 2018-03-21  Pedro Franco de Carvalho  <pedromfc@linux.vnet.ibm.com>
 
 	* MAINTAINERS (Write After Approval): Add Pedro Franco de
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 4dd9286..b1ba005 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -430,6 +430,7 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/scoped_fd-selftests.c \
 	unittests/scoped_mmap-selftests.c \
 	unittests/scoped_restore-selftests.c \
+	unittests/tracepoint-selftests.c \
 	unittests/unpack-selftests.c \
 	unittests/utils-selftests.c \
 	unittests/xml-utils-selftests.c
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 30fe206..8e779bc 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -3720,12 +3720,20 @@ parse_static_tracepoint_marker_definition (const char *line, const char **pp,
   p += 2 * end;
   p++;  /* skip a colon */
 
-  marker->extra = (char *) xmalloc (strlen (p) + 1);
-  end = hex2bin (p, (gdb_byte *) marker->extra, strlen (p) / 2);
+  /* This definition may be followed by another one, separated by a comma.  */
+  int hex_len;
+  endp = strchr (p, ',');
+  if (endp != nullptr)
+    hex_len = endp - p;
+  else
+    hex_len = strlen (p);
+
+  marker->extra = (char *) xmalloc (hex_len / 2 + 1);
+  end = hex2bin (p, (gdb_byte *) marker->extra, hex_len / 2);
   marker->extra[end] = '\0';
 
   if (pp)
-    *pp = p;
+    *pp = p + hex_len;
 }
 
 /* Release a static tracepoint marker's contents.  Note that the
diff --git a/gdb/unittests/tracepoint-selftests.c b/gdb/unittests/tracepoint-selftests.c
new file mode 100644
index 0000000..21ca3d0
--- /dev/null
+++ b/gdb/unittests/tracepoint-selftests.c
@@ -0,0 +1,70 @@
+/* Self tests for tracepoint-related code for GDB, the GNU debugger.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "selftest.h"
+#include "tracepoint.h"
+
+namespace selftests {
+namespace tracepoint_tests {
+
+static void
+test_parse_static_tracepoint_marker_definition ()
+{
+  static_tracepoint_marker marker;
+  const char def[] = ("1234:6d61726b657231:6578747261207374756666,"
+		      "abba:6d61726b657232:,"
+		      "cafe:6d61726b657233:6d6f72657374756666");
+  const char *start = def;
+  const char *end;
+
+  parse_static_tracepoint_marker_definition (start, &end, &marker);
+
+  SELF_CHECK (marker.address == 0x1234);
+  SELF_CHECK (strcmp (marker.str_id, "marker1") == 0);
+  SELF_CHECK (strcmp (marker.extra, "extra stuff") == 0);
+  SELF_CHECK (end == strchr (start, ','));
+
+  start = end + 1;
+  parse_static_tracepoint_marker_definition (start, &end, &marker);
+
+  SELF_CHECK (marker.address == 0xabba);
+  SELF_CHECK (strcmp (marker.str_id, "marker2") == 0);
+  SELF_CHECK (strcmp (marker.extra, "") == 0);
+  SELF_CHECK (end == strchr (start, ','));
+
+  start = end + 1;
+  parse_static_tracepoint_marker_definition (start, &end, &marker);
+
+  SELF_CHECK (marker.address == 0xcafe);
+  SELF_CHECK (strcmp (marker.str_id, "marker3") == 0);
+  SELF_CHECK (strcmp (marker.extra, "morestuff") == 0);
+  SELF_CHECK (end == def + strlen (def));
+}
+
+} /* namespace tracepoint_tests */
+} /* namespace selftests */
+
+void
+_initialize_tracepoint_selftests ()
+{
+  selftests::register_test
+    ("parse_static_tracepoint_marker_definition",
+     selftests::tracepoint_tests::test_parse_static_tracepoint_marker_definition);
+}