linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Yordan Karadzhov <y.karadz@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it
Date: Fri, 14 May 2021 16:45:51 +0300	[thread overview]
Message-ID: <29e3a3e1-26fe-a7e2-fec0-605794e2f353@gmail.com> (raw)
In-Reply-To: <20210514093133.504d1008@gandalf.local.home>



On 14.05.21 г. 16:31, Steven Rostedt wrote:
> On Fri, 14 May 2021 15:18:25 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> When closing a "tep" data stream we destroy the "trace_seq" object.
>> However, trace_seq_destroy() sets the buffer to "TRACE_SEQ_POISON"
>> which is different from NULL.
>>
>> It is unfortunate that TRACE_SEQ_POISON is an internal definition
>> of libtraceevent, so we have to redefine it here, but this can be
>> fixed in the future.
> 
> It's not unfortunate. It can change in the future without breaking API.
> 
> Redefining it here is not robust, and if trace-seq decides to do something
> different with that poison value, this will break, and it can't be blamed
> on API (using internal knowledge to implement code is not protected by
> being backward compatible).
> 
> The correct solution is to NULL the buffer after calling destroy.
> 
> 	if (seq.buffer) {
> 		trace_seq_destroy(&seq);
> 		seq.buffer = NULL;
> 	}

This was the first fix I did when I found the problem, but I don't like 
it because it looks like a hack the the user of the library is doing to 
trick the internal logic of the library.

Why not just moving the definition of TRACE_SEQ_POISON to the header or 
adding

book trace_seq_is_destroyed()

After this we can remove the clone from here.

Thanks!
Yordan

> 
> Just like you would do with a pointer you free but may use NULL as a value
> you are checking.
> 
> -- Steve
> 
> 
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   src/libkshark-tepdata.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
>> index bc5babb..4a84141 100644
>> --- a/src/libkshark-tepdata.c
>> +++ b/src/libkshark-tepdata.c
>> @@ -29,11 +29,17 @@
>>   #include "libkshark-plugin.h"
>>   #include "libkshark-tepdata.h"
>>   
>> +/**
>> + * The TEP_SEQ_POISON is to catch the use of
>> + * a trace_seq structure after it was destroyed.
>> + */
>> +#define TEP_SEQ_POISON	((void *)0xdeadbeef)
>> +
>>   static __thread struct trace_seq seq;
>>   
>>   static bool init_thread_seq(void)
>>   {
>> -	if (!seq.buffer)
>> +	if (!seq.buffer || seq.buffer == TEP_SEQ_POISON)
>>   		trace_seq_init(&seq);
>>   
>>   	return seq.buffer != NULL;
> 

  reply	other threads:[~2021-05-14 13:45 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-14 12:18 [PATCH 0/7] Final fixes before KS 2.0 Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 1/7] kernel-shark: Preserve markers when appending data Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 2/7] kernel-shark: Preserve open graphs " Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 3/7] kernel-shark: Clear before loading new session Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 4/7] kernel-shark: Better handling of plugins when appending data file Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 5/7] kernel-shark: Do draw the combo point of the mark Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it Yordan Karadzhov (VMware)
2021-05-14 13:31   ` Steven Rostedt
2021-05-14 13:45     ` Yordan Karadzhov [this message]
2021-05-14 13:57       ` Steven Rostedt
2021-05-14 14:01         ` Yordan Karadzhov
2021-05-14 12:18 ` [PATCH 7/7] kernel-shark: Quiet the warning printing from libtraceevent Yordan Karadzhov (VMware)
2021-05-14 13:33   ` Steven Rostedt
2021-05-14 17:45 ` [PATCH 0/7] Final fixes before KS 2.0 Steven Rostedt
2021-05-14 18:01 ` Steven Rostedt
2021-05-17 10:22   ` Yordan Karadzhov
2021-05-17 12:54     ` Steven Rostedt
2021-05-17 13:04       ` Yordan Karadzhov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=29e3a3e1-26fe-a7e2-fec0-605794e2f353@gmail.com \
    --to=y.karadz@gmail.com \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --subject='Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).