Re: [PATCH/commit] Handle errors in tracepoint target agent

[Pedro Alves <pedro@codesourcery.com>]

(could you please remember to put an empty line between
quotes and replies, so everyone else's eyes don't get
hurt?  Thanks in advance.  :-) )

On Friday 26 March 2010 12:50:23, Stan Shebs wrote:
> > But now, `error_desc' is leaking in the case you xmalloc it.
> >   
> It was leaking before. :-)  It needs to stick around indefinitely; it 
> guess it would work to free an old one just before allocating a new one.
> > When you fix that leak, you'll stumble of the fact that

I meant now, compared to before any patch was applied, of
course.  Word games are unfair!  :-)

> > while in others you have this:
> >
> >  ts->error_desc = (char *) xmalloc (p2 - p1 + 1);
> >   
> No, because the p2 != p1 test prior ensures we only allocate non-empty 
> strings.
> > How can you reliably know if you may xfree
> > ts->error_desc that way?  xfree ("") is a no go.
> > Comparing a pointer with "" is also not good.
> >   
> strlen.

My turn to triple yuck.  :-)

Would you object to this instead?

error_desc should only ever be accessed if the stopping reason
is tracepoint_error.

I've checked that all callers of parse_trace_status
currently only pass it the global status object.  If we
end up building an object of this type on the stack, we'll
need to memset it.  That would also be a requirement if we
checked for strlen instead..

-- 
Pedro Alves
2010-03-26  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* tracepoint.c (current_trace_status): Don't make sure error_desc
	is non-NULL here.
	(parse_trace_status): Release a previous error_desc string, and
	set it to NULL by default.  If stop reason is tracepoint_error, make
	sure error_desc is not left NULL.

---
 gdb/tracepoint.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2010-03-26 14:32:01.000000000 +0000
+++ src/gdb/tracepoint.c	2010-03-26 14:38:31.000000000 +0000
@@ -202,9 +202,6 @@ char *stop_reason_names[] = {
 struct trace_status *
 current_trace_status ()
 {
-  /* Ensure this is never NULL.  */
-  if (!trace_status.error_desc)
-    trace_status.error_desc = "";
   return &trace_status;
 }
 
@@ -3157,7 +3154,8 @@ parse_trace_status (char *line, struct t
   ts->running_known = 1;
   ts->running = (*p++ == '1');
   ts->stop_reason = trace_stop_reason_unknown;
-  ts->error_desc = "";
+  xfree (ts->error_desc);
+  ts->error_desc = NULL;
   ts->traceframe_count = -1;
   ts->traceframes_created = -1;
   ts->buffer_free = -1;
@@ -3201,6 +3199,9 @@ Status line: '%s'\n"), p, line);
 	      end = hex2bin (p1, ts->error_desc, (p2 - p1) / 2);
 	      ts->error_desc[end] = '\0';
 	    }
+	  else
+	    ts->error_desc = xstrdup ("");
+
 	  p = unpack_varlen_hex (++p2, &val);
 	  ts->stopping_tracepoint = val;
 	  ts->stop_reason = tracepoint_error;