[PATCH] trace-cmd: Check if file version is supported

[PATCH] trace-cmd: Check if file version is supported

[Tzvetomir Stoyanov (VMware)]

When reading a trace file, version of the file is ignored. This could
case problems when bumping the version number because of changes in
in the structure of the file. The old code should detect unsupported
file version and should not try to read it.
A new trace-cmd library API is added to check if version is supported:
 tracecmd_is_version_supported()
Checks are added in the code to ensure not trying to read trace file
with unsupported version.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/include/private/trace-cmd-private.h |  2 ++
 lib/trace-cmd/trace-input.c                       | 10 ++++++++++
 lib/trace-cmd/trace-util.c                        |  8 ++++++++
 tracecmd/trace-dump.c                             |  7 +++++++
 4 files changed, 27 insertions(+)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 56f82244..f7c1fa10 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -42,6 +42,8 @@ void tracecmd_record_ref(struct tep_record *record);
 void tracecmd_set_debug(bool set_debug);
 bool tracecmd_get_debug(void);
 
+bool tracecmd_is_version_supported(unsigned int version);
+
 struct tracecmd_output;
 struct tracecmd_recorder;
 struct hook_list;
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 991abd5f..9007c44e 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -117,6 +117,7 @@ struct tracecmd_input {
 	bool			use_trace_clock;
 	bool			read_page;
 	bool			use_pipe;
+	int			file_version;
 	struct cpu_data 	*cpu_data;
 	long long		ts_offset;
 	struct tsc2nsec		tsc_calc;
@@ -3175,6 +3176,7 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
 	unsigned int page_size;
 	char *version;
 	char buf[BUFSIZ];
+	long ver;
 
 	handle = malloc(sizeof(*handle));
 	if (!handle)
@@ -3199,6 +3201,14 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
 	if (!version)
 		goto failed_read;
 	pr_stat("version = %s\n", version);
+	ver = strtol(version, NULL, 10);
+	if (ver > INT_MAX || ver < INT_MIN || (!ver && errno))
+		goto failed_read;
+	if (!tracecmd_is_version_supported(ver)) {
+		tracecmd_warning("Unsupported file version %d", ver);
+		goto failed_read;
+	}
+	handle->file_version = ver;
 	free(version);
 
 	if (do_read_check(handle, buf, 1))
diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
index 2d3bc741..bacc47d1 100644
--- a/lib/trace-cmd/trace-util.c
+++ b/lib/trace-cmd/trace-util.c
@@ -582,3 +582,11 @@ unsigned long long tracecmd_generate_traceid(void)
 	free(str);
 	return hash;
 }
+
+bool tracecmd_is_version_supported(unsigned int version)
+{
+	if (version < FILE_VERSION)
+		return true;
+	return false;
+}
+
diff --git a/tracecmd/trace-dump.c b/tracecmd/trace-dump.c
index 3f56f65a..8e74d8f5 100644
--- a/tracecmd/trace-dump.c
+++ b/tracecmd/trace-dump.c
@@ -10,6 +10,7 @@
 #include <getopt.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <errno.h>
 
 #include "trace-local.h"
 
@@ -145,6 +146,7 @@ static void dump_initial_format(int fd)
 	char magic[] = TRACECMD_MAGIC;
 	char buf[DUMP_SIZE];
 	int val4;
+	long ver;
 
 	do_print(SUMMARY, "\t[Initial format]\n");
 
@@ -166,6 +168,11 @@ static void dump_initial_format(int fd)
 		die("no version string");
 
 	do_print(SUMMARY, "\t\t%s\t[Version]\n", buf);
+	ver = strtol(buf, NULL, 10);
+	if (ver > INT_MAX || ver < INT_MIN || (!ver && errno))
+		die("Invalid file version string");
+	if (!tracecmd_is_version_supported(ver))
+		die("Unsupported file version %d", ver);
 
 	/* get file endianness*/
 	if (read_file_bytes(fd, buf, 1))
-- 
2.30.2

Re: [PATCH] trace-cmd: Check if file version is supported

[Steven Rostedt]

On Fri, 16 Apr 2021 16:31:10 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> When reading a trace file, version of the file is ignored. This could
> case problems when bumping the version number because of changes in
> in the structure of the file. The old code should detect unsupported
> file version and should not try to read it.
> A new trace-cmd library API is added to check if version is supported:
>  tracecmd_is_version_supported()
> Checks are added in the code to ensure not trying to read trace file
> with unsupported version.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/include/private/trace-cmd-private.h |  2 ++
>  lib/trace-cmd/trace-input.c                       | 10 ++++++++++
>  lib/trace-cmd/trace-util.c                        |  8 ++++++++
>  tracecmd/trace-dump.c                             |  7 +++++++
>  4 files changed, 27 insertions(+)
> 
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index 56f82244..f7c1fa10 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -42,6 +42,8 @@ void tracecmd_record_ref(struct tep_record *record);
>  void tracecmd_set_debug(bool set_debug);
>  bool tracecmd_get_debug(void);
>  
> +bool tracecmd_is_version_supported(unsigned int version);
> +
>  struct tracecmd_output;
>  struct tracecmd_recorder;
>  struct hook_list;
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 991abd5f..9007c44e 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -117,6 +117,7 @@ struct tracecmd_input {
>  	bool			use_trace_clock;
>  	bool			read_page;
>  	bool			use_pipe;
> +	int			file_version;
>  	struct cpu_data 	*cpu_data;
>  	long long		ts_offset;
>  	struct tsc2nsec		tsc_calc;
> @@ -3175,6 +3176,7 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
>  	unsigned int page_size;
>  	char *version;
>  	char buf[BUFSIZ];
> +	long ver;

If we make this a unsigned long, we don't need all the checks.

>  
>  	handle = malloc(sizeof(*handle));
>  	if (!handle)
> @@ -3199,6 +3201,14 @@ struct tracecmd_input *tracecmd_alloc_fd(int fd, int flags)
>  	if (!version)
>  		goto failed_read;
>  	pr_stat("version = %s\n", version);
> +	ver = strtol(version, NULL, 10);
> +	if (ver > INT_MAX || ver < INT_MIN || (!ver && errno))

if it is unsigned, then we don't need to worry about negative numbers, and
you could just have:

	if (!ver && errno)


> +		goto failed_read;
> +	if (!tracecmd_is_version_supported(ver)) {

And have this take an unsigned long as well.

> +		tracecmd_warning("Unsupported file version %d", ver);

If ver is unsigned long, it should be: %lu instead of %d.

> +		goto failed_read;
> +	}
> +	handle->file_version = ver;
>  	free(version);
>  
>  	if (do_read_check(handle, buf, 1))
> diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
> index 2d3bc741..bacc47d1 100644
> --- a/lib/trace-cmd/trace-util.c
> +++ b/lib/trace-cmd/trace-util.c
> @@ -582,3 +582,11 @@ unsigned long long tracecmd_generate_traceid(void)
>  	free(str);
>  	return hash;
>  }
> +
> +bool tracecmd_is_version_supported(unsigned int version)

Have the above be unsigned long to match versions. Who knows, maybe some
day this will need to handle a file version greater than 4 billion :-)

And by then, there would be no more 32 bit machines.


> +{
> +	if (version < FILE_VERSION)

I think you want "<=" as with this patch I have:

trace-cmd report
  Unsupported file version 6
  error reading header for trace.dat

Better yet, just make it:

	return version <= FILE_VERSION;



> +		return true;
> +	return false;
> +}
> +

Extra whitespace at the end of the file. git complains about it.

> diff --git a/tracecmd/trace-dump.c b/tracecmd/trace-dump.c
> index 3f56f65a..8e74d8f5 100644
> --- a/tracecmd/trace-dump.c
> +++ b/tracecmd/trace-dump.c
> @@ -10,6 +10,7 @@
>  #include <getopt.h>
>  #include <sys/stat.h>
>  #include <fcntl.h>
> +#include <errno.h>
>  
>  #include "trace-local.h"
>  
> @@ -145,6 +146,7 @@ static void dump_initial_format(int fd)
>  	char magic[] = TRACECMD_MAGIC;
>  	char buf[DUMP_SIZE];
>  	int val4;
> +	long ver;
>  
>  	do_print(SUMMARY, "\t[Initial format]\n");
>  
> @@ -166,6 +168,11 @@ static void dump_initial_format(int fd)
>  		die("no version string");
>  
>  	do_print(SUMMARY, "\t\t%s\t[Version]\n", buf);
> +	ver = strtol(buf, NULL, 10);
> +	if (ver > INT_MAX || ver < INT_MIN || (!ver && errno))

Again, make it unsigned long and you wont need all these INT_MAX INT_MIN
checks. If it's > INT_MAX, then the versioning would fail.

> +		die("Invalid file version string");
> +	if (!tracecmd_is_version_supported(ver))
> +		die("Unsupported file version %d", ver);
>  
>  	/* get file endianness*/
>  	if (read_file_bytes(fd, buf, 1))


-- Steve