diff mbox

[3/3] Fix tracepoint.c:parse_tracepoint_definition leak (and one more)

Message ID 20181214191242.21560-4-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Dec. 14, 2018, 7:12 p.m. UTC 
  Coverity points out that gdb/tracepoint.c:parse_tracepoint_definition
can leak 'cond' in this line:

      cond = (char *) xmalloc (2 * xlen + 1);

That can leak because we're in a loop and 'cond' may have already been
xmalloc'ed into in a previous iteration.  That won't normally happen,
because we don't expect to see a tracepoint definition with multiple
conditions listed, but, it doesn't hurt to be pedantically correct,
in case some stub manages to send something odd back to GDB.

At first I thought I'd just replace the xmalloc call with:

      cond = (char *) xrealloc (cond, 2 * xlen + 1);

and be done with it.  However, my pedantic self realizes that
warning() can throw as well (due to pagination + Ctrl-C), so I fixed
it using gdb::unique_xmalloc_ptr instead.

While doing this, I noticed that these vectors in struct uploaded_tp:

  std::vector<char *> actions;
  std::vector<char *> step_actions;

hold heap-allocated strings, but nothing is freeing the strings,
AFAICS.

So I ended up switching all the heap-allocated strings in uploaded_tp
to unique pointers.  This patch is the result of that.

I also wrote an alternative, but similar patch that uses std::string
throughout instead of gdb::unique_xmalloc_ptr, but in the end reverted
it because the code didn't look that much better, and I kind of
dislike replacing pointers with fat std::string's (3 or 4 times the
size of a pointer) in structures.  Let me know if you have a strong
preference otherwise.

gdb/ChangeLog:
2018-12-14  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (read_uploaded_action)
	(create_tracepoint_from_upload): Adjust to use
	gdb::unique_xmalloc_ptr.
	* ctf.c (ctf_write_uploaded_tp):
	(SET_ARRAY_FIELD): Use emplace_back.
	(SET_STRING_FIELD): Adjust to use gdb::unique_xmalloc_ptr.
	* tracefile-tfile.c (tfile_write_uploaded_tp):
	* tracepoint.c (parse_tracepoint_definition): Adjust to use
	gdb::unique_xmalloc_ptr.
	* tracepoint.h (struct uploaded_tp) <cond, actions, step_actions,
	at_string, cond_string, cmd_strings>: Replace char pointers
	with gdb::unique_xmalloc_ptr.
---
 gdb/breakpoint.c      |  6 +++---
 gdb/ctf.c             | 30 +++++++++++++++++-------------
 gdb/tracefile-tfile.c | 21 +++++++++++----------
 gdb/tracepoint.c      | 24 +++++++++++++-----------
 gdb/tracepoint.h      | 12 ++++++------
 5 files changed, 50 insertions(+), 43 deletions(-)

Comments

Simon Marchi Dec. 14, 2018, 11:07 p.m. UTC | #1 
On 2018-12-14 14:12, Pedro Alves wrote:
> Coverity points out that gdb/tracepoint.c:parse_tracepoint_definition
> can leak 'cond' in this line:
> 
>       cond = (char *) xmalloc (2 * xlen + 1);
> 
> That can leak because we're in a loop and 'cond' may have already been
> xmalloc'ed into in a previous iteration.  That won't normally happen,
> because we don't expect to see a tracepoint definition with multiple
> conditions listed, but, it doesn't hurt to be pedantically correct,
> in case some stub manages to send something odd back to GDB.
> 
> At first I thought I'd just replace the xmalloc call with:
> 
>       cond = (char *) xrealloc (cond, 2 * xlen + 1);
> 
> and be done with it.  However, my pedantic self realizes that
> warning() can throw as well (due to pagination + Ctrl-C), so I fixed
> it using gdb::unique_xmalloc_ptr instead.
> 
> While doing this, I noticed that these vectors in struct uploaded_tp:
> 
>   std::vector<char *> actions;
>   std::vector<char *> step_actions;
> 
> hold heap-allocated strings, but nothing is freeing the strings,
> AFAICS.
> 
> So I ended up switching all the heap-allocated strings in uploaded_tp
> to unique pointers.  This patch is the result of that.
> 
> I also wrote an alternative, but similar patch that uses std::string
> throughout instead of gdb::unique_xmalloc_ptr, but in the end reverted
> it because the code didn't look that much better, and I kind of
> dislike replacing pointers with fat std::string's (3 or 4 times the
> size of a pointer) in structures.  Let me know if you have a strong
> preference otherwise.

I think I've tried to fix that using std::string in the past and 
probably didn't find the result satisfactory either, so I'm fine with 
this patch.

I just have one question:

> diff --git a/gdb/ctf.c b/gdb/ctf.c
> index ca5266fbd7..e98fb9c1ba 100644
> --- a/gdb/ctf.c
> +++ b/gdb/ctf.c
> @@ -596,38 +596,42 @@ ctf_write_uploaded_tp (struct trace_file_writer 
> *self,
> 
>    /* condition  */
>    if (tp->cond != NULL)
> -    ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond, strlen 
> (tp->cond));
> +    ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond.get (),
> +		    strlen (tp->cond.get ()));
>    ctf_save_write (&writer->tcs, &zero, 1);
> 
>    /* actions */
>    u32 = tp->actions.size ();
>    ctf_save_align_write (&writer->tcs, (gdb_byte *) &u32, 4, 4);
> -  for (char *act : tp->actions)
> -    ctf_save_write (&writer->tcs, (gdb_byte *) act, strlen (act) + 1);
> +  for (auto &&act : tp->actions)
> +    ctf_save_write (&writer->tcs, (gdb_byte *) act.get (),
> +		    strlen (act.get ()) + 1);

Is there a reason to use "auto &&", instead of let's say "const auto &"? 
  Since we don't want to modify the unique_ptr...

I just read:

   
https://blog.petrzemek.net/2016/08/17/auto-type-deduction-in-range-based-for-loops/
   
https://isocpp.org/blog/2012/11/universal-references-in-c11-scott-meyers

and didn't immediately see why it would be useful in this situation, so 
I thought I'd ask.

Simon
Pedro Alves Jan. 10, 2019, 6:07 p.m. UTC | #2 
On 12/14/2018 11:07 PM, Simon Marchi wrote:
> On 2018-12-14 14:12, Pedro Alves wrote:

> I just have one question:
> 
>> diff --git a/gdb/ctf.c b/gdb/ctf.c
>> index ca5266fbd7..e98fb9c1ba 100644
>> --- a/gdb/ctf.c
>> +++ b/gdb/ctf.c
>> @@ -596,38 +596,42 @@ ctf_write_uploaded_tp (struct trace_file_writer *self,
>>
>>    /* condition  */
>>    if (tp->cond != NULL)
>> -    ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond, strlen (tp->cond));
>> +    ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond.get (),
>> +            strlen (tp->cond.get ()));
>>    ctf_save_write (&writer->tcs, &zero, 1);
>>
>>    /* actions */
>>    u32 = tp->actions.size ();
>>    ctf_save_align_write (&writer->tcs, (gdb_byte *) &u32, 4, 4);
>> -  for (char *act : tp->actions)
>> -    ctf_save_write (&writer->tcs, (gdb_byte *) act, strlen (act) + 1);
>> +  for (auto &&act : tp->actions)
>> +    ctf_save_write (&writer->tcs, (gdb_byte *) act.get (),
>> +            strlen (act.get ()) + 1);
> 
> Is there a reason to use "auto &&", instead of let's say "const auto &"?  Since we don't want to modify the unique_ptr...
> 
> I just read:
> 
>   https://blog.petrzemek.net/2016/08/17/auto-type-deduction-in-range-based-for-loops/
>   https://isocpp.org/blog/2012/11/universal-references-in-c11-scott-meyers
> 
> and didn't immediately see why it would be useful in this situation, so I thought I'd ask.

No reason, I had just written auto&& without thinking about
it, mainly because auto&& universally works.

I changed it to 'const auto &' and pushed in the series.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8af3d54a77..4a0b8dc0dc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14681,7 +14681,7 @@  read_uploaded_action (void)
 
   if (next_cmd < this_utp->cmd_strings.size ())
     {
-      rslt = this_utp->cmd_strings[next_cmd];
+      rslt = this_utp->cmd_strings[next_cmd].get ();
       next_cmd++;
     }
 
@@ -14702,7 +14702,7 @@  create_tracepoint_from_upload (struct uploaded_tp *utp)
   struct tracepoint *tp;
 
   if (utp->at_string)
-    addr_str = utp->at_string;
+    addr_str = utp->at_string.get ();
   else
     {
       /* In the absence of a source location, fall back to raw
@@ -14726,7 +14726,7 @@  create_tracepoint_from_upload (struct uploaded_tp *utp)
 							 current_language);
   if (!create_breakpoint (get_current_arch (),
 			  location.get (),
-			  utp->cond_string, -1, addr_str,
+			  utp->cond_string.get (), -1, addr_str,
 			  0 /* parse cond/thread */,
 			  0 /* tempflag */,
 			  utp->type /* type_wanted */,
diff --git a/gdb/ctf.c b/gdb/ctf.c
index ca5266fbd7..e98fb9c1ba 100644
--- a/gdb/ctf.c
+++ b/gdb/ctf.c
@@ -596,38 +596,42 @@  ctf_write_uploaded_tp (struct trace_file_writer *self,
 
   /* condition  */
   if (tp->cond != NULL)
-    ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond, strlen (tp->cond));
+    ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond.get (),
+		    strlen (tp->cond.get ()));
   ctf_save_write (&writer->tcs, &zero, 1);
 
   /* actions */
   u32 = tp->actions.size ();
   ctf_save_align_write (&writer->tcs, (gdb_byte *) &u32, 4, 4);
-  for (char *act : tp->actions)
-    ctf_save_write (&writer->tcs, (gdb_byte *) act, strlen (act) + 1);
+  for (auto &&act : tp->actions)
+    ctf_save_write (&writer->tcs, (gdb_byte *) act.get (),
+		    strlen (act.get ()) + 1);
 
   /* step_actions */
   u32 = tp->step_actions.size ();
   ctf_save_align_write (&writer->tcs, (gdb_byte *) &u32, 4, 4);
-  for (char *act : tp->step_actions)
-    ctf_save_write (&writer->tcs, (gdb_byte *) act, strlen (act) + 1);
+  for (auto &&act : tp->step_actions)
+    ctf_save_write (&writer->tcs, (gdb_byte *) act.get (),
+		    strlen (act.get ()) + 1);
 
   /* at_string */
   if (tp->at_string != NULL)
-    ctf_save_write (&writer->tcs, (gdb_byte *) tp->at_string,
-		    strlen (tp->at_string));
+    ctf_save_write (&writer->tcs, (gdb_byte *) tp->at_string.get (),
+		    strlen (tp->at_string.get ()));
   ctf_save_write (&writer->tcs, &zero, 1);
 
   /* cond_string */
   if (tp->cond_string != NULL)
-    ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond_string,
-		    strlen (tp->cond_string));
+    ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond_string.get (),
+		    strlen (tp->cond_string.get ()));
   ctf_save_write (&writer->tcs, &zero, 1);
 
   /* cmd_strings */
   u32 = tp->cmd_strings.size ();
   ctf_save_align_write (&writer->tcs, (gdb_byte *) &u32, 4, 4);
-  for (char *act : tp->cmd_strings)
-    ctf_save_write (&writer->tcs, (gdb_byte *) act, strlen (act) + 1);
+  for (auto &&act : tp->cmd_strings)
+    ctf_save_write (&writer->tcs, (gdb_byte *) act.get (),
+		    strlen (act.get ()) + 1);
 
 }
 
@@ -1023,7 +1027,7 @@  ctf_read_tsv (struct uploaded_tsv **uploaded_tsvs)
 	  const struct bt_definition *element				\
 	    = bt_ctf_get_index ((EVENT), def, i);			\
 									\
-	  (VAR)->ARRAY.push_back					\
+	  (VAR)->ARRAY.emplace_back					\
 	    (xstrdup (bt_ctf_get_string (element)));			\
 	}								\
     }									\
@@ -1040,7 +1044,7 @@  ctf_read_tsv (struct uploaded_tsv **uploaded_tsvs)
 							   #FIELD));	\
 									\
       if (strlen (p) > 0)						\
-	(VAR)->FIELD = xstrdup (p);					\
+	(VAR)->FIELD.reset (xstrdup (p));				\
       else								\
 	(VAR)->FIELD = NULL;						\
     }									\
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 6996e3d1e5..8e8cb15fd4 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -262,31 +262,32 @@  tfile_write_uploaded_tp (struct trace_file_writer *self,
     fprintf (writer->fp, ":F%x", utp->orig_size);
   if (utp->cond)
     fprintf (writer->fp,
-	     ":X%x,%s", (unsigned int) strlen (utp->cond) / 2,
-	     utp->cond);
+	     ":X%x,%s", (unsigned int) strlen (utp->cond.get ()) / 2,
+	     utp->cond.get ());
   fprintf (writer->fp, "\n");
-  for (char *act : utp->actions)
+  for (auto &&act : utp->actions)
     fprintf (writer->fp, "tp A%x:%s:%s\n",
-	     utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act);
-  for (char *act : utp->step_actions)
+	     utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act.get ());
+  for (auto &&act : utp->step_actions)
     fprintf (writer->fp, "tp S%x:%s:%s\n",
-	     utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act);
+	     utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act.get ());
   if (utp->at_string)
     {
       encode_source_string (utp->number, utp->addr,
-			    "at", utp->at_string, buf, MAX_TRACE_UPLOAD);
+			    "at", utp->at_string.get (),
+			    buf, MAX_TRACE_UPLOAD);
       fprintf (writer->fp, "tp Z%s\n", buf);
     }
   if (utp->cond_string)
     {
       encode_source_string (utp->number, utp->addr,
-			    "cond", utp->cond_string,
+			    "cond", utp->cond_string.get (),
 			    buf, MAX_TRACE_UPLOAD);
       fprintf (writer->fp, "tp Z%s\n", buf);
     }
-  for (char *act : utp->cmd_strings)
+  for (auto &&act : utp->cmd_strings)
     {
-      encode_source_string (utp->number, utp->addr, "cmd", act,
+      encode_source_string (utp->number, utp->addr, "cmd", act.get (),
 			    buf, MAX_TRACE_UPLOAD);
       fprintf (writer->fp, "tp Z%s\n", buf);
     }
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 8cd53374f9..4c940b0014 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -3083,7 +3083,8 @@  find_matching_tracepoint_location (struct uploaded_tp *utp)
       if (b->type == utp->type
 	  && t->step_count == utp->step
 	  && t->pass_count == utp->pass
-	  && cond_string_is_same (t->cond_string, utp->cond_string)
+	  && cond_string_is_same (t->cond_string,
+				  utp->cond_string.get ())
 	  /* FIXME also test actions.  */
 	  )
 	{
@@ -3462,7 +3463,7 @@  parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp)
   int enabled, end;
   enum bptype type;
   const char *srctype;
-  char *cond, *buf;
+  char *buf;
   struct uploaded_tp *utp = NULL;
 
   p = line;
@@ -3475,13 +3476,14 @@  parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp)
   p++;  /* skip a colon */
   if (piece == 'T')
     {
+      gdb::unique_xmalloc_ptr<char[]> cond;
+
       enabled = (*p++ == 'E');
       p++;  /* skip a colon */
       p = unpack_varlen_hex (p, &step);
       p++;  /* skip a colon */
       p = unpack_varlen_hex (p, &pass);
       type = bp_tracepoint;
-      cond = NULL;
       /* Thumb through optional fields.  */
       while (*p == ':')
 	{
@@ -3502,8 +3504,8 @@  parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp)
 	      p++;
 	      p = unpack_varlen_hex (p, &xlen);
 	      p++;  /* skip a comma */
-	      cond = (char *) xmalloc (2 * xlen + 1);
-	      strncpy (cond, p, 2 * xlen);
+	      cond.reset ((char *) xmalloc (2 * xlen + 1));
+	      strncpy (&cond[0], p, 2 * xlen);
 	      cond[2 * xlen] = '\0';
 	      p += 2 * xlen;
 	    }
@@ -3516,17 +3518,17 @@  parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp)
       utp->enabled = enabled;
       utp->step = step;
       utp->pass = pass;
-      utp->cond = cond;
+      utp->cond = std::move (cond);
     }
   else if (piece == 'A')
     {
       utp = get_uploaded_tp (num, addr, utpp);
-      utp->actions.push_back (xstrdup (p));
+      utp->actions.emplace_back (xstrdup (p));
     }
   else if (piece == 'S')
     {
       utp = get_uploaded_tp (num, addr, utpp);
-      utp->step_actions.push_back (xstrdup (p));
+      utp->step_actions.emplace_back (xstrdup (p));
     }
   else if (piece == 'Z')
     {
@@ -3546,11 +3548,11 @@  parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp)
       buf[end] = '\0';
 
       if (startswith (srctype, "at:"))
-	utp->at_string = xstrdup (buf);
+	utp->at_string.reset (xstrdup (buf));
       else if (startswith (srctype, "cond:"))
-	utp->cond_string = xstrdup (buf);
+	utp->cond_string.reset (xstrdup (buf));
       else if (startswith (srctype, "cmd:"))
-	utp->cmd_strings.push_back (xstrdup (buf));
+	utp->cmd_strings.emplace_back (xstrdup (buf));
     }
   else if (piece == 'V')
     {
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index b43fbd7606..fbde53ebf4 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -178,21 +178,21 @@  struct uploaded_tp
   int orig_size = 0;
 
   /* String that is the encoded form of the tracepoint's condition.  */
-  char *cond = nullptr;
+  gdb::unique_xmalloc_ptr<char[]> cond;
 
   /* Vectors of strings that are the encoded forms of a tracepoint's
      actions.  */
-  std::vector<char *> actions;
-  std::vector<char *> step_actions;
+  std::vector<gdb::unique_xmalloc_ptr<char[]>> actions;
+  std::vector<gdb::unique_xmalloc_ptr<char[]>> step_actions;
 
   /* The original string defining the location of the tracepoint.  */
-  char *at_string = nullptr;
+  gdb::unique_xmalloc_ptr<char[]> at_string;
 
   /* The original string defining the tracepoint's condition.  */
-  char *cond_string = nullptr;
+  gdb::unique_xmalloc_ptr<char[]> cond_string;
 
   /* List of original strings defining the tracepoint's actions.  */
-  std::vector<char *> cmd_strings;
+  std::vector<gdb::unique_xmalloc_ptr<char[]>> cmd_strings;
 
   /* The tracepoint's current hit count.  */
   int hit_count = 0;