[PATCH v2] tracefs events: Do not process str_read_file() if size is zero

[PATCH v2] tracefs events: Do not process str_read_file() if size is zero

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

If a file has no size (nothing is read), then str_read_file() frees the
buffer and returns zero. The problem is that all callers of str_read_file()
uses the buffer supplied if the value returned is not a negative. This
causes the freed buffer being used by the callers if the file read existed
but had no content.

This is apparent when using a copy of the tracefs directory, where some file
exist, but have no content, then loading the events would cause a segfault.

Change the callers to check the return value of str_read_file() for zero or
negative, and do not go further if it is.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
Changes since v1:

   Have the callers of str_read_file() check for less than zero, as
   the first patch would never allocate the passed in buffer.

 tracefs-events.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tracefs-events.c b/tracefs-events.c
index ca1d22b..631c310 100644
--- a/tracefs-events.c
+++ b/tracefs-events.c
@@ -411,7 +411,7 @@ char **tracefs_tracers(const char *tracing_dir)
 		goto out_free;
 
 	len = str_read_file(available_tracers, &buf);
-	if (len < 0)
+	if (len <= 0)
 		goto out_free;
 
 	len = 0;
@@ -471,7 +471,7 @@ static int load_events(struct tep_handle *tep,
 			goto next_event;
 
 		len = str_read_file(format, &buf);
-		if (len < 0)
+		if (len <= 0)
 			goto next_event;
 
 		ret = tep_parse_event(tep, buf, len, system);
@@ -501,7 +501,7 @@ static int read_header(struct tep_handle *tep, const char *tracing_dir)
 		goto out;
 
 	len = str_read_file(header, &buf);
-	if (len < 0)
+	if (len <= 0)
 		goto out;
 
 	tep_parse_header_page(tep, buf, len, sizeof(long));
-- 
2.25.4

Re: [PATCH v2] tracefs events: Do not process str_read_file() if size is zero

[Tzvetomir Stoyanov]

On Mon, Nov 23, 2020 at 10:50 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> If a file has no size (nothing is read), then str_read_file() frees the
> buffer and returns zero. The problem is that all callers of str_read_file()
> uses the buffer supplied if the value returned is not a negative. This
> causes the freed buffer being used by the callers if the file read existed
> but had no content.
>
> This is apparent when using a copy of the tracefs directory, where some file
> exist, but have no content, then loading the events would cause a segfault.
>
> Change the callers to check the return value of str_read_file() for zero or
> negative, and do not go further if it is.
>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> Changes since v1:
>
>    Have the callers of str_read_file() check for less than zero, as
>    the first patch would never allocate the passed in buffer.
>
>  tracefs-events.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tracefs-events.c b/tracefs-events.c
> index ca1d22b..631c310 100644
> --- a/tracefs-events.c
> +++ b/tracefs-events.c
> @@ -411,7 +411,7 @@ char **tracefs_tracers(const char *tracing_dir)
>                 goto out_free;
>
>         len = str_read_file(available_tracers, &buf);
> -       if (len < 0)
> +       if (len <= 0)
>                 goto out_free;
>
>         len = 0;
> @@ -471,7 +471,7 @@ static int load_events(struct tep_handle *tep,
>                         goto next_event;
>
>                 len = str_read_file(format, &buf);
> -               if (len < 0)
> +               if (len <= 0)
>                         goto next_event;
>
>                 ret = tep_parse_event(tep, buf, len, system);
> @@ -501,7 +501,7 @@ static int read_header(struct tep_handle *tep, const char *tracing_dir)
>                 goto out;
>
>         len = str_read_file(header, &buf);
> -       if (len < 0)
> +       if (len <= 0)
>                 goto out;
>
>         tep_parse_header_page(tep, buf, len, sizeof(long));
> --
> 2.25.4
>
Thanks, looks good to me.
Acked-by: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>


--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center