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

[Simon Marchi]

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