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

Yordan Karadzhov <y.karadz@xxxxxxxxx> · Fri, 14 May 2021 16:45:51 +0300

On 14.05.21 г. 16:31, Steven Rostedt wrote:

On Fri, 14 May 2021 15:18:25 +0300
"Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> 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@xxxxxxxxx>
---
  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;





