[PATCH 0/2] Build and fuzzing related fixes

[PATCH 0/2] Build and fuzzing related fixes

[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

[PATCH 1/2] libtraceevent: Add eof checks.

[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

Re: [PATCH 1/2] libtraceevent: Add eof checks.

[Steven Rostedt]

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

Re: [PATCH 1/2] libtraceevent: Add eof checks.

[Steven Rostedt]

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

[PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.

[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

Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.

[Steven Rostedt]

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, ...);

Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.

[Ian Rogers]

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, ...);
>

Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.

[Steven Rostedt]

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

Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.

[Ian Rogers]

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

Re: [PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.

[Steven Rostedt]

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

[PATCH 1/2] libtraceevent: Add eof checks.

[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

[PATCH 2/2] libtraceevent: Changed angled brackets to double quotes.

[Claire Jensen]

Certain build systems, such as bazel, are picky about angled brackets vs
double quotes.

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.

Signed-off-by: Claire Jensen <cjense@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.288.g62a8d224e6-goog