From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21267 invoked by alias); 26 Mar 2010 14:44:22 -0000 Received: (qmail 21259 invoked by uid 22791); 26 Mar 2010 14:44:22 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 26 Mar 2010 14:44:18 +0000 Received: (qmail 16692 invoked from network); 26 Mar 2010 14:44:16 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 26 Mar 2010 14:44:16 -0000 From: Pedro Alves To: Stan Shebs Subject: Re: [PATCH/commit] Handle errors in tracepoint target agent Date: Fri, 26 Mar 2010 14:44:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <4BAC1426.5050003@codesourcery.com> <201003261140.09071.pedro@codesourcery.com> <4BACAD8F.3030909@codesourcery.com> In-Reply-To: <4BACAD8F.3030909@codesourcery.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201003261444.14280.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-03/txt/msg00888.txt.bz2 (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 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;