Re: [PATCH 13/38] trace-cmd record: prevent a memory leak in show_error()

On 17/07/2024 22:51, Steven Rostedt wrote:
> On Wed,  5 Jun 2024 15:40:28 +0200
> "Jerome Marchand" <[email protected]> wrote:
> 
>> In show_error() the pointer p is used for several functions. At some
>> point it is used to contain the error log file. It's not freed before
>> being replaced by the result of read_file(path), which is not freed
>> either. Free p in both case.
> 
> This isn't that big of a deal as this is just an application that is
> about to exit anyway. But I'm fine with freeing it. That said:
> 
>>
>> Fixes a RESOURCE_LEAK error (CWE-772)
>>
>> Signed-off-by: Jerome Marchand <[email protected]>
>> ---
>>   tracecmd/trace-record.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
>> index f05a58d1..3e29f922 100644
>> --- a/tracecmd/trace-record.c
>> +++ b/tracecmd/trace-record.c
>> @@ -2364,13 +2364,16 @@ static void show_error(const char *file,
>> const char *type) goto read_file;
>>   		}
>>   		read_error_log(p);
>> +		free(p);
>>   		goto out;
>>   	}
>>   
>>    read_file:
>>   	p = read_file(path);
>> -	if (p)
>> +	if (p) {
>>   		printf("%s", p);
>> +		free(p);
>> +	}
>>   
>>    out:
> 
> It would be much cleaner to just add:
> 
> 	free(p);
> 
> here, and remove the above two calls.

Good point. I'll update the description which is incorrect too.

Jerome

> 
>>   	printf("Failed %s of %s\n", type, file);
> 
> I'll skip this patch too.
> 
> -- Steve