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 16:45:51 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:

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

It's not a hack. seq.buffer is exposed via the API (it's in the header) and
is allowed to be used (we use it all the time).

The trace_seq_destroy() function is only to clean up everything that
trace_seq_init() had done, and the seq is no longer valid, and the user is
free to do whatever they want with it afterward. Like set seq.buffer to
NULL.

This is a perfectly valid use case.


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

Because TRACE_SEQ_POISON is an internal API that I never want to expose,
because I may even change it in the future. After destroy is called, the
trace_seq code is done. If you want to use the trace_seq again, you need to
call init.

Technically, if you want to do it prim and proper (but I don't actually
recommend this), you need to have another variable that keeps track of the
seq if it was allocated or not. That's not the responsibility of the
trace_seq API to do so. All the trace_seq API cares about is the time
trace_seq_init() is called till trace_seq_destroy() is called. Before or
after that, the seq is of no use to it.

You would need to have:

bool seq_is_init;

	if (!seq_is_init) {
		trace_seq_init(&seq);
		seq_is_init = true;
	}

and later

	if (seq_is_init) {
		trace_seq_destroy(&seq);
		seq_is_init = false;
	}

that's if you don't want to use seq.buffer == NULL to do that for you.

-- Steve