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

[Steven Rostedt <rostedt@goodmis.org>]

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

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;