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

[Steven Rostedt <rostedt@goodmis.org>]

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