Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it

[Yordan Karadzhov <y.karadz@gmail.com>]

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;
>