Skip to contentSkip to conversation

tools lib traceevent: Fix kbuffer_read_at_offset()

PATCH In review Namhyung Kim 4 participants 6 messages Oct 1, 2016 – Oct 7, 2016

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 tools lib traceevent: Fix kbuffer_read_at_offset()

When it's called with an offset less than or equal to the first event, it'll return a garbage value since the data is not initialized.

Cc Steven Rostedt <[email protected]>
---
 tools/lib/traceevent/kbuffer-parse.c | 1 +
 1 file changed, 1 insertion(+)
tools/lib/traceevent/kbuffer-parse.c +1 -0
index 3bcada3ae05a..65984f1c2974 100644
@@ -622,6 +622,7 @@ void *kbuffer_read_at_offset(struct kbuffer *kbuf, int offset,
 
 	/* Reset the buffer */
 	kbuffer_load_subbuffer(kbuf, kbuf->subbuffer);
+	data = kbuffer_read_event(kbuf, ts);
 
 	while (kbuf->curr < offset) {
 		data = kbuffer_next_event(kbuf, ts);
Signature · 1 line
2.10.0
#1 lore raw

Em Sat, Oct 01, 2016 at 07:17:00PM +0900, Namhyung Kim escreveu:

Quoted 2 lines

When it's called with an offset less than or equal to the first event, it'll return a garbage value since the data is not initialized.

Steven, Ack?

- Arnaldo

Quoted 20 lines
Cc Steven Rostedt <[email protected]>
---
 tools/lib/traceevent/kbuffer-parse.c | 1 +
 1 file changed, 1 insertion(+)
tools/lib/traceevent/kbuffer-parse.c +1 -0
index 3bcada3ae05a..65984f1c2974 100644
@@ -622,6 +622,7 @@ void *kbuffer_read_at_offset(struct kbuffer *kbuf, int offset,
 
 	/* Reset the buffer */
 	kbuffer_load_subbuffer(kbuf, kbuf->subbuffer);
+	data = kbuffer_read_event(kbuf, ts);
 
 	while (kbuf->curr < offset) {
 		data = kbuffer_next_event(kbuf, ts);
Signature · 1 line
2.10.0
Acked-by on v1 1/1
#2 lore raw
In reply to Namhyung Kim
Quoted 2 lines · On Sat, 1 Oct 2016 19:17:00 +0900 Namhyung Kim <[email protected]> wrote:

When it's called with an offset less than or equal to the first event, it'll return a garbage value since the data is not initialized.

Well, it can at most be equal to (unless offset is negative) because kbuffer_load_subbuffer() sets kbuf->curr to zero.

But that said, it looks like offset == 0 is buggy.

-- Steve

Quoted 19 lines
Cc Steven Rostedt <[email protected]>
---
 tools/lib/traceevent/kbuffer-parse.c | 1 +
 1 file changed, 1 insertion(+)
tools/lib/traceevent/kbuffer-parse.c +1 -0
index 3bcada3ae05a..65984f1c2974 100644
@@ -622,6 +622,7 @@ void *kbuffer_read_at_offset(struct kbuffer *kbuf, int offset,
 
 	/* Reset the buffer */
 	kbuffer_load_subbuffer(kbuf, kbuf->subbuffer);
+	data = kbuffer_read_event(kbuf, ts);
 
 	while (kbuf->curr < offset) {
 		data = kbuffer_next_event(kbuf, ts);
#3 lore raw

Hi Steve,

Quoted 8 lines · Steven Rostedt
Quoted 2 lines · On Sat, 1 Oct 2016 19:17:00 +0900 Namhyung Kim <[email protected]> wrote:

When it's called with an offset less than or equal to the first event, it'll return a garbage value since the data is not initialized.

Well, it can at most be equal to (unless offset is negative) because kbuffer_load_subbuffer() sets kbuf->curr to zero.

Actually kbuffer_load_subbuffer() calls kbuf->next_event(). Inside the function it has a loop updating next valid event. Sometimes, the data starts with TIME_EXTEND with value of 0 and the loop skips it which ended up setting kbuf->curr to 8. :)

I'll take a look it later.

Quoted 4 lines

But that said, it looks like offset == 0 is buggy.

Thanks,
Namhyung

Quoted 24 lines

-- Steve

Quoted 19 lines
Cc Steven Rostedt <[email protected]>
---
 tools/lib/traceevent/kbuffer-parse.c | 1 +
 1 file changed, 1 insertion(+)
tools/lib/traceevent/kbuffer-parse.c +1 -0
index 3bcada3ae05a..65984f1c2974 100644
@@ -622,6 +622,7 @@ void *kbuffer_read_at_offset(struct kbuffer *kbuf, int offset,
 
 	/* Reset the buffer */
 	kbuffer_load_subbuffer(kbuf, kbuf->subbuffer);
+	data = kbuffer_read_event(kbuf, ts);
 
 	while (kbuf->curr < offset) {
 		data = kbuffer_next_event(kbuf, ts);
#4 lore raw
Quoted 4 lines · On Fri, 7 Oct 2016 13:19:08 +0900 Namhyung Kim <[email protected]> wrote:

Actually kbuffer_load_subbuffer() calls kbuf->next_event(). Inside the function it has a loop updating next valid event. Sometimes, the data starts with TIME_EXTEND with value of 0 and the loop skips it which ended up setting kbuf->curr to 8. :)

Right, because kbuffer_read_at_offset() should not return a TIME_EXTEND.

-- Steve

#5 lore raw
In reply to Namhyung Kim
tools lib traceevent: Fix kbuffer_read_at_offset()
Commit-ID:  a130347973c408c0e0017a52c74cbc3226c1f0ef
Gitweb:     http://git.kernel.org/tip/a130347973c408c0e0017a52c74cbc3226c1f0ef
Author:     Namhyung Kim <[email protected]>
AuthorDate: Sat, 1 Oct 2016 19:17:00 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 5 Oct 2016 11:36:22 -0300

tools lib traceevent: Fix kbuffer_read_at_offset()

When it's called with an offset less than or equal to the first event, it'll return a garbage value since the data is not initialized.

---
 tools/lib/traceevent/kbuffer-parse.c | 1 +
 1 file changed, 1 insertion(+)
tools/lib/traceevent/kbuffer-parse.c +1 -0
index 3bcada3..65984f1 100644
@@ -622,6 +622,7 @@ void *kbuffer_read_at_offset(struct kbuffer *kbuf, int offset,
 
 	/* Reset the buffer */
 	kbuffer_load_subbuffer(kbuf, kbuf->subbuffer);
+	data = kbuffer_read_event(kbuf, ts);
 
 	while (kbuf->curr < offset) {
 		data = kbuffer_next_event(kbuf, ts);