linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Build and fuzzing related fixes
@ 2021-06-12  1:44 Ian Rogers
  2021-06-12  1:45 ` [PATCH 1/2] libtraceevent: Add eof checks Ian Rogers
  2021-06-12  1:45 ` [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes Ian Rogers
  0 siblings, 2 replies; 15+ messages in thread
From: Ian Rogers @ 2021-06-12  1:44 UTC (permalink / raw)
  To: linux-trace-devel, Tzvetomir Stoyanov, Steven Rostedt, Claire Jensen
  Cc: Ian Rogers

EOF checks are missing in a number of cases that cause the parser to
enter an infinite loop if an EOF is encountered.
Some build systems are picky about angled vs quotes, fix this minor issue.

Claire Jensen (2):
  libtraceevent: Add eof checks.
  libtraceevent: Changed angled brackets to double quotes.

 src/event-parse.c | 62 ++++++++++++++++++++++++++++++++++++-----------
 src/event-utils.h |  2 +-
 2 files changed, 49 insertions(+), 15 deletions(-)

-- 
2.32.0.272.g935e593368-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] libtraceevent: Add eof checks.
  2021-06-12  1:44 [PATCH 0/2] Build and fuzzing related fixes Ian Rogers
@ 2021-06-12  1:45 ` Ian Rogers
  2021-06-13 23:30   ` Steven Rostedt
  2021-06-12  1:45 ` [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes Ian Rogers
  1 sibling, 1 reply; 15+ messages in thread
From: Ian Rogers @ 2021-06-12  1:45 UTC (permalink / raw)
  To: linux-trace-devel, Tzvetomir Stoyanov, Steven Rostedt, Claire Jensen
  Cc: Ian Rogers

From: Claire Jensen <cjense@google.com>

Added checking for __read_char and peek_char to make sure value is not at end
of file.

Signed-off-by: Claire Jensen <cjense@google.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 src/event-parse.c | 62 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/src/event-parse.c b/src/event-parse.c
index 9915cb4..ac11887 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -1155,17 +1155,16 @@ static enum tep_event_type force_token(const char *str, char **tok);
 static enum tep_event_type __read_token(char **tok)
 {
 	char buf[BUFSIZ];
-	int ch, last_ch, quote_ch, next_ch;
+	int ch, last_ch, quote_ch, next_ch, read_ch, peek_ch;
 	int i = 0;
 	int tok_size = 0;
 	enum tep_event_type type;
 
 	*tok = NULL;
 
-
-	ch = __read_char();
+        ch = __read_char();
 	if (ch < 0)
-		return TEP_EVENT_NONE;
+		goto out_eof_error;
 
 	type = get_type(ch);
 	if (type == TEP_EVENT_NONE)
@@ -1184,9 +1183,15 @@ static enum tep_event_type __read_token(char **tok)
 	case TEP_EVENT_OP:
 		switch (ch) {
 		case '-':
-			next_ch = peek_char();
+			peek_ch = peek_char();
+			if (peek_ch < 0)
+				goto out_eof_error;
+			next_ch = peek_ch;
 			if (next_ch == '>') {
-				buf[i++] = __read_char();
+				read_ch = __read_char();
+				if (read_ch < 0)
+					goto out_eof_error;
+				buf[i++] = read_ch;
 				break;
 			}
 			/* fall through */
@@ -1197,9 +1202,14 @@ static enum tep_event_type __read_token(char **tok)
 		case '<':
 			last_ch = ch;
 			ch = peek_char();
+			if (ch < 0)
+				goto out_eof_error;
 			if (ch != last_ch)
 				goto test_equal;
-			buf[i++] = __read_char();
+			read_ch = __read_char();
+			if (read_ch < 0)
+				goto out_eof_error;
+			buf[i++] = read_ch;
 			switch (last_ch) {
 			case '>':
 			case '<':
@@ -1219,10 +1229,17 @@ static enum tep_event_type __read_token(char **tok)
 		return type;
 
  test_equal:
-		ch = peek_char();
-		if (ch == '=')
-			buf[i++] = __read_char();
-		goto out;
+		peek_ch = peek_char();
+		if (peek_ch < 0)
+			goto out_eof_error;
+		ch = peek_ch;
+		if (ch == '=') {
+			read_ch = __read_char();
+			if (read_ch < 0)
+				goto out_eof_error;
+			buf[i++] = read_ch;
+			goto out;
+		}
 
 	case TEP_EVENT_DQUOTE:
 	case TEP_EVENT_SQUOTE:
@@ -1242,6 +1259,8 @@ static enum tep_event_type __read_token(char **tok)
 			}
 			last_ch = ch;
 			ch = __read_char();
+			if(ch < 0)
+				goto out_eof_error;
 			buf[i++] = ch;
 			/* the '\' '\' will cancel itself */
 			if (ch == '\\' && last_ch == '\\')
@@ -1259,6 +1278,8 @@ static enum tep_event_type __read_token(char **tok)
 
 			do {
 				ch = __read_char();
+				if(ch < 0)
+					return TEP_EVENT_NONE;
 			} while (isspace(ch));
 			if (ch == '"')
 				goto concat;
@@ -1273,7 +1294,13 @@ static enum tep_event_type __read_token(char **tok)
 		break;
 	}
 
-	while (get_type(peek_char()) == type) {
+	while (1) {
+		peek_ch = peek_char();
+		if (peek_ch < 0)
+			goto out_eof_error;
+		if (get_type(peek_ch) != type)
+			break;
+
 		if (i == (BUFSIZ - 1)) {
 			buf[i] = 0;
 			tok_size += BUFSIZ;
@@ -1282,8 +1309,10 @@ static enum tep_event_type __read_token(char **tok)
 				return TEP_EVENT_NONE;
 			i = 0;
 		}
-		ch = __read_char();
-		buf[i++] = ch;
+		read_ch = __read_char();
+		if (read_ch < 0)
+			goto out_eof_error;
+		buf[i++] = read_ch;
 	}
 
  out:
@@ -1316,6 +1345,11 @@ static enum tep_event_type __read_token(char **tok)
 	}
 
 	return type;
+
+out_eof_error:
+	free(*tok);
+	*tok = NULL;
+	return TEP_EVENT_NONE;
 }
 
 static enum tep_event_type force_token(const char *str, char **tok)
-- 
2.32.0.272.g935e593368-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.
  2021-06-12  1:44 [PATCH 0/2] Build and fuzzing related fixes Ian Rogers
  2021-06-12  1:45 ` [PATCH 1/2] libtraceevent: Add eof checks Ian Rogers
@ 2021-06-12  1:45 ` Ian Rogers
  2021-06-12 23:00   ` Steven Rostedt
  1 sibling, 1 reply; 15+ messages in thread
From: Ian Rogers @ 2021-06-12  1:45 UTC (permalink / raw)
  To: linux-trace-devel, Tzvetomir Stoyanov, Steven Rostedt, Claire Jensen
  Cc: Ian Rogers

From: Claire Jensen <cjense@google.com>

Signed-off-by: Claire Jensen <cjense@google.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 src/event-utils.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/event-utils.h b/src/event-utils.h
index d377201..44f7968 100644
--- a/src/event-utils.h
+++ b/src/event-utils.h
@@ -10,7 +10,7 @@
 #include <stdarg.h>
 #include <stdbool.h>
 
-#include <event-parse.h>
+#include "event-parse.h"
 
 void tep_warning(const char *fmt, ...);
 void tep_info(const char *fmt, ...);
-- 
2.32.0.272.g935e593368-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.
  2021-06-12  1:45 ` [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes Ian Rogers
@ 2021-06-12 23:00   ` Steven Rostedt
  2021-06-13 21:31     ` Ian Rogers
  0 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2021-06-12 23:00 UTC (permalink / raw)
  To: Ian Rogers; +Cc: linux-trace-devel, Tzvetomir Stoyanov, Claire Jensen

On Fri, 11 Jun 2021 18:45:01 -0700
Ian Rogers <irogers@google.com> wrote:

> From: Claire Jensen <cjense@google.com>
> 
> Signed-off-by: Claire Jensen <cjense@google.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  src/event-utils.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/event-utils.h b/src/event-utils.h
> index d377201..44f7968 100644
> --- a/src/event-utils.h
> +++ b/src/event-utils.h
> @@ -10,7 +10,7 @@
>  #include <stdarg.h>
>  #include <stdbool.h>
>  
> -#include <event-parse.h>
> +#include "event-parse.h"

Thanks for the update, but event-parse.h is part of the exported
library and even though it happens to be local to this file, we use the
'<' and '>' to denote that it is a library header and not a local one.

-- Steve


>  
>  void tep_warning(const char *fmt, ...);
>  void tep_info(const char *fmt, ...);


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.
  2021-06-12 23:00   ` Steven Rostedt
@ 2021-06-13 21:31     ` Ian Rogers
  2021-06-13 23:29       ` Steven Rostedt
  0 siblings, 1 reply; 15+ messages in thread
From: Ian Rogers @ 2021-06-13 21:31 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Linux Trace Devel, Tzvetomir Stoyanov, Claire Jensen

On Sat, Jun 12, 2021 at 4:00 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 11 Jun 2021 18:45:01 -0700
> Ian Rogers <irogers@google.com> wrote:
>
> > From: Claire Jensen <cjense@google.com>
> >
> > Signed-off-by: Claire Jensen <cjense@google.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  src/event-utils.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/event-utils.h b/src/event-utils.h
> > index d377201..44f7968 100644
> > --- a/src/event-utils.h
> > +++ b/src/event-utils.h
> > @@ -10,7 +10,7 @@
> >  #include <stdarg.h>
> >  #include <stdbool.h>
> >
> > -#include <event-parse.h>
> > +#include "event-parse.h"
>
> Thanks for the update, but event-parse.h is part of the exported
> library and even though it happens to be local to this file, we use the
> '<' and '>' to denote that it is a library header and not a local one.
>
> -- Steve

Would it be more conventional in this situation to use quotes within
the library and angles outside? This change is within the library and
not with code trying to link against it.

Thanks,
Ian

>
> >
> >  void tep_warning(const char *fmt, ...);
> >  void tep_info(const char *fmt, ...);
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.
  2021-06-13 21:31     ` Ian Rogers
@ 2021-06-13 23:29       ` Steven Rostedt
  2021-06-14 16:14         ` Ian Rogers
  0 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2021-06-13 23:29 UTC (permalink / raw)
  To: Ian Rogers; +Cc: Linux Trace Devel, Tzvetomir Stoyanov, Claire Jensen

On Sun, 13 Jun 2021 14:31:42 -0700
Ian Rogers <irogers@google.com> wrote:


> Would it be more conventional in this situation to use quotes within
> the library and angles outside? This change is within the library and
> not with code trying to link against it.

Does it matter? If not, what's the purpose of changing it?

Note, the change log is empty, and is where rationale for the change is
suppose to be added.

-- Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] libtraceevent: Add eof checks.
  2021-06-12  1:45 ` [PATCH 1/2] libtraceevent: Add eof checks Ian Rogers
@ 2021-06-13 23:30   ` Steven Rostedt
       [not found]     ` <CAFPGG2iQK8XMv6Z1-KurgjOnYuk=m=uWNWJXj6OEb_SBQkokZA@mail.gmail.com>
  0 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2021-06-13 23:30 UTC (permalink / raw)
  To: Ian Rogers; +Cc: linux-trace-devel, Tzvetomir Stoyanov, Claire Jensen

On Fri, 11 Jun 2021 18:45:00 -0700
Ian Rogers <irogers@google.com> wrote:

> From: Claire Jensen <cjense@google.com>
> 
> Added checking for __read_char and peek_char to make sure value is not at end
> of file.

Was there a case where this happened?

-- Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.
  2021-06-13 23:29       ` Steven Rostedt
@ 2021-06-14 16:14         ` Ian Rogers
  2021-06-14 18:43           ` Steven Rostedt
  0 siblings, 1 reply; 15+ messages in thread
From: Ian Rogers @ 2021-06-14 16:14 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Linux Trace Devel, Tzvetomir Stoyanov, Claire Jensen

On Sun, Jun 13, 2021 at 4:29 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Sun, 13 Jun 2021 14:31:42 -0700
> Ian Rogers <irogers@google.com> wrote:
>
>
> > Would it be more conventional in this situation to use quotes within
> > the library and angles outside? This change is within the library and
> > not with code trying to link against it.
>
> Does it matter? If not, what's the purpose of changing it?

Sorry for not having this in the body, in the cover message I added to
Claire's email with:

> Some build systems are picky about angled vs quotes, fix this minor issue.

The problem is with certain build system, we use bazel at Google,
where iquote is used for includes within the library, and other
flavors outside the library. There's a similar conversation here:
https://groups.google.com/g/bazel-discuss/c/6MNuZ5bKoa8?pli=1

the failure you see when building is:

In file included from src/event-parse.c:27:
src/event-utils.h:13:10: error: 'event-parse.h' file not found with
<angled> include; use "quotes" instead
#include <event-parse.h>
         ^~~~~~~~~~~~~~~
         "event-parse.h"

Arguably -iquote is more correct within the library build, hence bazel
using it. The libtraceevent Makefile is using -I, hence this not being
a problem for others. We prefer not to carry internal patches, hence
wanting to send this upstream.

Thanks,
Ian

> Note, the change log is empty, and is where rationale for the change is
> suppose to be added.
>
> -- Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.
  2021-06-14 16:14         ` Ian Rogers
@ 2021-06-14 18:43           ` Steven Rostedt
  0 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2021-06-14 18:43 UTC (permalink / raw)
  To: Ian Rogers; +Cc: Linux Trace Devel, Tzvetomir Stoyanov, Claire Jensen

On Mon, 14 Jun 2021 09:14:08 -0700
Ian Rogers <irogers@google.com> wrote:

> On Sun, Jun 13, 2021 at 4:29 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Sun, 13 Jun 2021 14:31:42 -0700
> > Ian Rogers <irogers@google.com> wrote:
> >
> >  
> > > Would it be more conventional in this situation to use quotes within
> > > the library and angles outside? This change is within the library and
> > > not with code trying to link against it.  
> >
> > Does it matter? If not, what's the purpose of changing it?  
> 
> Sorry for not having this in the body, in the cover message I added to
> Claire's email with:
> 
> > Some build systems are picky about angled vs quotes, fix this minor issue.  
> 
> The problem is with certain build system, we use bazel at Google,
> where iquote is used for includes within the library, and other
> flavors outside the library. There's a similar conversation here:
> https://groups.google.com/g/bazel-discuss/c/6MNuZ5bKoa8?pli=1
> 
> the failure you see when building is:
> 
> In file included from src/event-parse.c:27:
> src/event-utils.h:13:10: error: 'event-parse.h' file not found with
> <angled> include; use "quotes" instead
> #include <event-parse.h>
>          ^~~~~~~~~~~~~~~
>          "event-parse.h"
> 
> Arguably -iquote is more correct within the library build, hence bazel
> using it. The libtraceevent Makefile is using -I, hence this not being
> a problem for others. We prefer not to carry internal patches, hence
> wanting to send this upstream.

OK, context is very important :-)

With the above explanation, it makes sense, and I'm OK with taking it.
Please resend with the above in the commit log.

Thanks!

-- Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] libtraceevent: Add eof checks.
       [not found]     ` <CAFPGG2iQK8XMv6Z1-KurgjOnYuk=m=uWNWJXj6OEb_SBQkokZA@mail.gmail.com>
@ 2021-06-17 19:01       ` Steven Rostedt
  0 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2021-06-17 19:01 UTC (permalink / raw)
  To: Claire Jensen; +Cc: Ian Rogers, linux-trace-devel, Tzvetomir Stoyanov

On Thu, 17 Jun 2021 13:56:49 -0500
Claire Jensen <cjense@google.com> wrote:

> Hi Steve,
> 
> This issue was found while fuzz testing. One of the test cases created an
> infinite loop because __read_token had reached end of file. Checking was
> added to all cases where this may occur.
> 
> Sorry for the late reply, I just got an issue surrounding email permissions
> resolved so I was unable to respond earlier.
> 
>

Can you resend the patch with this information in the change log.

Thanks!

-- Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] libtraceevent: Add eof checks.
  2021-06-24  5:50     ` Ian Rogers
@ 2021-06-24 13:14       ` Steven Rostedt
  0 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2021-06-24 13:14 UTC (permalink / raw)
  To: Ian Rogers; +Cc: Claire Jensen, eranian, tz.stoyanov, linux-trace-devel

On Wed, 23 Jun 2021 22:50:58 -0700
Ian Rogers <irogers@google.com> wrote:

> Hi Steve,
> 
> sorry for the breakage, could you give a full reproduction command?

You can download this: http://rostedt.org/private/trace-issue.dat

And run trace-cmd report --debug on it, and you'll see that it fails to
parse with the patch, but works fine without it.

-- Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] libtraceevent: Add eof checks.
  2021-06-24  1:06   ` Steven Rostedt
@ 2021-06-24  5:50     ` Ian Rogers
  2021-06-24 13:14       ` Steven Rostedt
  0 siblings, 1 reply; 15+ messages in thread
From: Ian Rogers @ 2021-06-24  5:50 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Claire Jensen, eranian, tz.stoyanov, linux-trace-devel

On Wed, Jun 23, 2021 at 6:06 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 17 Jun 2021 15:58:28 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Thu, 17 Jun 2021 19:43:25 +0000
> > Claire Jensen <cjense@google.com> wrote:
> >
> > Hi Claire,
> >
> > Thanks for sending the patches, I'll try to get some time to look at them
> > (note, that I have a lot of other duties that I need to finish before I can
> > get to this).
> >
> > > Added checking for __read_char and peek_char to make sure value is not at end
> > > of file.
> > >
> > > This issue was found while fuzz testing. One of the test cases created an infinite loop because __read_token had reached end of file. Checking was added to all cases where this may occur.
> >
> > You don't need to fix this now, but for future reference, we follow the
> > Linux guidelines on submitting patches which includes having line breaks at
> > 74 (although I use 76) bytes, for the change log.
> >
> > This makes the change logs easier to read.
> >
>
> I made the mistake of adding this patch and pushing it to a new release
> without running my test suite against it. It ended up breaking the parsing.
>
> When running with --debug -N, I get:

Hi Steve,

sorry for the breakage, could you give a full reproduction command?

Thanks!
Ian

>   [ftrace:branch] unexpected type 1
>   [sched:sched_switch] unknown op ''
>   [irq:irq_handler_exit] unexpected type 1
>   [timer:timer_start] unknown op ''
>   [kvm:vcpu_match_mmio] unexpected type 1
>   [kvm:kvm_wait_lapic_expire] unknown op ''
>   [kvm:kvm_vcpu_wakeup] unexpected type 1
>   [kvm:kvm_userspace_exit] unknown op ''
>   [kvm:kvm_pv_tlb_flush] unexpected type 1
>   [kvm:kvm_ple_window_update] unknown op ''
>   [kvm:kvm_pio] unknown op ''
>   [kvm:kvm_pic_set_irq] unknown op ''
>   [kvm:kvm_nested_vmrun] unexpected type 1
>   [kvm:kvm_nested_vmexit_inject] unknown op ''
>   Error: expected type 5 but read 0
>   [kvm:kvm_nested_vmexit] unknown op ''
>   Error: expected type 5 but read 0
>   [kvm:kvm_nested_vmenter_failed] bad op token
>   [kvm:kvm_msr] unexpected type 1
>   [kvm:kvm_msi_set_irq] unknown op ''
>   unknown op ''
>   [kvm:kvm_ioapic_set_irq] unknown op ''
>   Error: expected type 5 but read 0
>   unknown op ''
>   [kvm:kvm_ioapic_delayed_eoi_inj] unknown op ''
>   Error: expected type 5 but read 0
>   [kvm:kvm_exit] unknown op ''
>   Error: expected type 5 but read 0
>   [kvm:kvm_emulate_insn] unknown op ''
>   Error: expected type 5 but read 0
>   [kvm:kvm_cpuid] unexpected type 1
>   unknown op ''
>   [kvm:kvm_apic_ipi] unknown op ''
>   Error: expected type 5 but read 0
>   unknown op ''
>   [kvm:kvm_apic_accept_irq] unknown op ''
>   Error: expected type 5 but read 0
>   [kvm:kvm_age_page] unexpected type 1
>
> with the patch, and no errors without it.
>
> I have to revert this patch and push a new version out.
>
> -- Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] libtraceevent: Add eof checks.
  2021-06-17 19:58 ` Steven Rostedt
@ 2021-06-24  1:06   ` Steven Rostedt
  2021-06-24  5:50     ` Ian Rogers
  0 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2021-06-24  1:06 UTC (permalink / raw)
  To: Claire Jensen; +Cc: eranian, irogers, tz.stoyanov, linux-trace-devel

On Thu, 17 Jun 2021 15:58:28 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 17 Jun 2021 19:43:25 +0000
> Claire Jensen <cjense@google.com> wrote:
> 
> Hi Claire,
> 
> Thanks for sending the patches, I'll try to get some time to look at them
> (note, that I have a lot of other duties that I need to finish before I can
> get to this).
> 
> > Added checking for __read_char and peek_char to make sure value is not at end
> > of file.
> > 
> > This issue was found while fuzz testing. One of the test cases created an infinite loop because __read_token had reached end of file. Checking was added to all cases where this may occur.  
> 
> You don't need to fix this now, but for future reference, we follow the
> Linux guidelines on submitting patches which includes having line breaks at
> 74 (although I use 76) bytes, for the change log.
> 
> This makes the change logs easier to read.
> 

I made the mistake of adding this patch and pushing it to a new release
without running my test suite against it. It ended up breaking the parsing.

When running with --debug -N, I get:

  [ftrace:branch] unexpected type 1
  [sched:sched_switch] unknown op ''
  [irq:irq_handler_exit] unexpected type 1
  [timer:timer_start] unknown op ''
  [kvm:vcpu_match_mmio] unexpected type 1
  [kvm:kvm_wait_lapic_expire] unknown op ''
  [kvm:kvm_vcpu_wakeup] unexpected type 1
  [kvm:kvm_userspace_exit] unknown op ''
  [kvm:kvm_pv_tlb_flush] unexpected type 1
  [kvm:kvm_ple_window_update] unknown op ''
  [kvm:kvm_pio] unknown op ''
  [kvm:kvm_pic_set_irq] unknown op ''
  [kvm:kvm_nested_vmrun] unexpected type 1
  [kvm:kvm_nested_vmexit_inject] unknown op ''
  Error: expected type 5 but read 0
  [kvm:kvm_nested_vmexit] unknown op ''
  Error: expected type 5 but read 0
  [kvm:kvm_nested_vmenter_failed] bad op token 
  [kvm:kvm_msr] unexpected type 1
  [kvm:kvm_msi_set_irq] unknown op ''
  unknown op ''
  [kvm:kvm_ioapic_set_irq] unknown op ''
  Error: expected type 5 but read 0
  unknown op ''
  [kvm:kvm_ioapic_delayed_eoi_inj] unknown op ''
  Error: expected type 5 but read 0
  [kvm:kvm_exit] unknown op ''
  Error: expected type 5 but read 0
  [kvm:kvm_emulate_insn] unknown op ''
  Error: expected type 5 but read 0
  [kvm:kvm_cpuid] unexpected type 1
  unknown op ''
  [kvm:kvm_apic_ipi] unknown op ''
  Error: expected type 5 but read 0
  unknown op ''
  [kvm:kvm_apic_accept_irq] unknown op ''
  Error: expected type 5 but read 0
  [kvm:kvm_age_page] unexpected type 1

with the patch, and no errors without it.

I have to revert this patch and push a new version out.

-- Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] libtraceevent: Add eof checks.
  2021-06-17 19:43 [PATCH 1/2] libtraceevent: Add eof checks Claire Jensen
@ 2021-06-17 19:58 ` Steven Rostedt
  2021-06-24  1:06   ` Steven Rostedt
  0 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2021-06-17 19:58 UTC (permalink / raw)
  To: Claire Jensen; +Cc: eranian, irogers, tz.stoyanov, linux-trace-devel

On Thu, 17 Jun 2021 19:43:25 +0000
Claire Jensen <cjense@google.com> wrote:

Hi Claire,

Thanks for sending the patches, I'll try to get some time to look at them
(note, that I have a lot of other duties that I need to finish before I can
get to this).

> Added checking for __read_char and peek_char to make sure value is not at end
> of file.
> 
> This issue was found while fuzz testing. One of the test cases created an infinite loop because __read_token had reached end of file. Checking was added to all cases where this may occur.

You don't need to fix this now, but for future reference, we follow the
Linux guidelines on submitting patches which includes having line breaks at
74 (although I use 76) bytes, for the change log.

This makes the change logs easier to read.

Thanks!

-- Steve


> 
> Signed-off-by: Claire Jensen <cjense@google.com>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] libtraceevent: Add eof checks.
@ 2021-06-17 19:43 Claire Jensen
  2021-06-17 19:58 ` Steven Rostedt
  0 siblings, 1 reply; 15+ messages in thread
From: Claire Jensen @ 2021-06-17 19:43 UTC (permalink / raw)
  To: eranian, irogers, tz.stoyanov, linux-trace-devel, rostedt; +Cc: Claire Jensen

Added checking for __read_char and peek_char to make sure value is not at end
of file.

This issue was found while fuzz testing. One of the test cases created an infinite loop because __read_token had reached end of file. Checking was added to all cases where this may occur.

Signed-off-by: Claire Jensen <cjense@google.com>
---
 src/event-parse.c | 62 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/src/event-parse.c b/src/event-parse.c
index 97c1a97..f454e23 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -1155,17 +1155,16 @@ static enum tep_event_type force_token(const char *str, char **tok);
 static enum tep_event_type __read_token(char **tok)
 {
 	char buf[BUFSIZ];
-	int ch, last_ch, quote_ch, next_ch;
+	int ch, last_ch, quote_ch, next_ch, read_ch, peek_ch;
 	int i = 0;
 	int tok_size = 0;
 	enum tep_event_type type;
 
 	*tok = NULL;
 
-
-	ch = __read_char();
+        ch = __read_char();
 	if (ch < 0)
-		return TEP_EVENT_NONE;
+		goto out_eof_error;
 
 	type = get_type(ch);
 	if (type == TEP_EVENT_NONE)
@@ -1184,9 +1183,15 @@ static enum tep_event_type __read_token(char **tok)
 	case TEP_EVENT_OP:
 		switch (ch) {
 		case '-':
-			next_ch = peek_char();
+			peek_ch = peek_char();
+			if (peek_ch < 0)
+				goto out_eof_error;
+			next_ch = peek_ch;
 			if (next_ch == '>') {
-				buf[i++] = __read_char();
+				read_ch = __read_char();
+				if (read_ch < 0)
+					goto out_eof_error;
+				buf[i++] = read_ch;
 				break;
 			}
 			/* fall through */
@@ -1197,9 +1202,14 @@ static enum tep_event_type __read_token(char **tok)
 		case '<':
 			last_ch = ch;
 			ch = peek_char();
+			if (ch < 0)
+				goto out_eof_error;
 			if (ch != last_ch)
 				goto test_equal;
-			buf[i++] = __read_char();
+			read_ch = __read_char();
+			if (read_ch < 0)
+				goto out_eof_error;
+			buf[i++] = read_ch;
 			switch (last_ch) {
 			case '>':
 			case '<':
@@ -1219,10 +1229,17 @@ static enum tep_event_type __read_token(char **tok)
 		return type;
 
  test_equal:
-		ch = peek_char();
-		if (ch == '=')
-			buf[i++] = __read_char();
-		goto out;
+		peek_ch = peek_char();
+		if (peek_ch < 0)
+			goto out_eof_error;
+		ch = peek_ch;
+		if (ch == '=') {
+			read_ch = __read_char();
+			if (read_ch < 0)
+				goto out_eof_error;
+			buf[i++] = read_ch;
+			goto out;
+		}
 
 	case TEP_EVENT_DQUOTE:
 	case TEP_EVENT_SQUOTE:
@@ -1242,6 +1259,8 @@ static enum tep_event_type __read_token(char **tok)
 			}
 			last_ch = ch;
 			ch = __read_char();
+			if(ch < 0)
+				goto out_eof_error;
 			buf[i++] = ch;
 			/* the '\' '\' will cancel itself */
 			if (ch == '\\' && last_ch == '\\')
@@ -1259,6 +1278,8 @@ static enum tep_event_type __read_token(char **tok)
 
 			do {
 				ch = __read_char();
+				if(ch < 0)
+					return TEP_EVENT_NONE;
 			} while (isspace(ch));
 			if (ch == '"')
 				goto concat;
@@ -1273,7 +1294,13 @@ static enum tep_event_type __read_token(char **tok)
 		break;
 	}
 
-	while (get_type(peek_char()) == type) {
+	while (1) {
+		peek_ch = peek_char();
+		if (peek_ch < 0)
+			goto out_eof_error;
+		if (get_type(peek_ch) != type)
+			break;
+
 		if (i == (BUFSIZ - 1)) {
 			buf[i] = 0;
 			tok_size += BUFSIZ;
@@ -1282,8 +1309,10 @@ static enum tep_event_type __read_token(char **tok)
 				return TEP_EVENT_NONE;
 			i = 0;
 		}
-		ch = __read_char();
-		buf[i++] = ch;
+		read_ch = __read_char();
+		if (read_ch < 0)
+			goto out_eof_error;
+		buf[i++] = read_ch;
 	}
 
  out:
@@ -1316,6 +1345,11 @@ static enum tep_event_type __read_token(char **tok)
 	}
 
 	return type;
+
+out_eof_error:
+	free(*tok);
+	*tok = NULL;
+	return TEP_EVENT_NONE;
 }
 
 static enum tep_event_type force_token(const char *str, char **tok)
-- 
2.32.0.288.g62a8d224e6-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-06-24 13:14 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-12  1:44 [PATCH 0/2] Build and fuzzing related fixes Ian Rogers
2021-06-12  1:45 ` [PATCH 1/2] libtraceevent: Add eof checks Ian Rogers
2021-06-13 23:30   ` Steven Rostedt
     [not found]     ` <CAFPGG2iQK8XMv6Z1-KurgjOnYuk=m=uWNWJXj6OEb_SBQkokZA@mail.gmail.com>
2021-06-17 19:01       ` Steven Rostedt
2021-06-12  1:45 ` [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes Ian Rogers
2021-06-12 23:00   ` Steven Rostedt
2021-06-13 21:31     ` Ian Rogers
2021-06-13 23:29       ` Steven Rostedt
2021-06-14 16:14         ` Ian Rogers
2021-06-14 18:43           ` Steven Rostedt
2021-06-17 19:43 [PATCH 1/2] libtraceevent: Add eof checks Claire Jensen
2021-06-17 19:58 ` Steven Rostedt
2021-06-24  1:06   ` Steven Rostedt
2021-06-24  5:50     ` Ian Rogers
2021-06-24 13:14       ` Steven Rostedt

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox