Re: [PATCH 23/38] trace-cmd lib: prevent buffer overrun in read_string()

[Steven Rostedt <rostedt@xxxxxxxxxxx>]

On Wed,  5 Jun 2024 15:40:38 +0200
"Jerome Marchand" <jmarchan@xxxxxxxxxx> wrote:


> 
> Fixes an OVERRUN error (CWE-119)
> 
> Signed-off-by: Jerome Marchand <jmarchan@xxxxxxxxxx>
> ---
>  lib/trace-cmd/trace-input.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 3284dbd4..c485acea 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -447,15 +447,13 @@ static char *read_string(struct tracecmd_input *handle)
>  		str = realloc(str, size);

Hmm, the above is a memory leak if it fails. That should be:

		tmp = realloc(str, size);
		if (!tmp) {
			free(str);
			return NULL;
		}
		str = tmp;

Not to do with your patch, but I think this code is generally wrong. :-/

Let's say BUFSIZ = 1024, and a string is 1124 in size, including the '\0'.

That is str[1123] = '\0'.

The loop would end with:

	size = 1024; i = 99;

	size += i + 1; // size = 1124;


>  		if (!str)
>  			return NULL;
> -		memcpy(str + (size - i), buf, i);
> -		str[size] = 0;
> +		memcpy(str + (size - i), buf, i + 1);

size - i = 1025

I think this is wrong, as we should be copying to str + 1024, and not str + 1025.

This should be fixed, but has nothing to do with this particular patch.

Thanks,

-- Steve


>  	} else {
>  		size = i + 1;
>  		str = malloc(size);
>  		if (!str)
>  			return NULL;
> -		memcpy(str, buf, i);
> -		str[i] = 0;
> +		memcpy(str, buf, i + 1);
>  	}
>  
>  	return str;