On Tue, 14 Sep 2021 16:12:22 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
> Some error paths in read_proc_kallsyms() may lead to a memory leak.
> Improved the error handling of this internal function to avoid it.
How so?
>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@xxxxxxxxx>
> ---
> lib/trace-cmd/trace-input.c | 39 +++++++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 32358ce9..9b23063e 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -739,34 +739,39 @@ static int read_event_files(struct tracecmd_input *handle, const char *regex)
>
> static int read_proc_kallsyms(struct tracecmd_input *handle)
> {
> - struct tep_handle *pevent = handle->pevent;
> + struct tep_handle *tep = handle->pevent;
> unsigned int size;
> - char *buf;
> + char *buf = NULL;
> + int ret;
>
> if (handle->file_state >= TRACECMD_FILE_KALLSYMS)
> return 0;
>
> - if (read4(handle, &size) < 0)
> - return -1;
> - if (!size)
> - return 0; /* OK? */
> + ret = read4(handle, &size);
> + if (ret < 0)
> + goto out;
> + if (!size) {
> + handle->file_state = TRACECMD_FILE_KALLSYMS;
> + goto out; /* OK? */
> + }
>
Allocation of buf happens below. There's no reason to jump to out. It
should just return here. Then you don't need to initialize buf.
> buf = malloc(size+1);
> - if (!buf)
> - return -1;
> - if (do_read_check(handle, buf, size)){
> - free(buf);
buf is freed here on the error path in the original code.
> - return -1;
> + if (!buf) {
> + ret = -1;
> + goto out;
The above doesn't even make sense. The only thing jumping to out does here
is to free buf, in which case, there is no buf to free.
> }
> - buf[size] = 0;
> -
> - tep_parse_kallsyms(pevent, buf);
> + ret = do_read_check(handle, buf, size);
> + if (ret < 0)
> + goto out;
>
> - free(buf);
> + buf[size] = 0;
> + tep_parse_kallsyms(tep, buf);
If you want to make another patch to do the slight updates to state that
this patch does (but does not express in the change log), that's fine. But
as this patch stands, it does not do what the change log states, so I'm
removing this patch from the series.
-- Steve
>
> handle->file_state = TRACECMD_FILE_KALLSYMS;
> -
> - return 0;
> + ret = 0;
> +out:
> + free(buf);
> + return ret;
> }
>
> static int read_ftrace_printk(struct tracecmd_input *handle)
|