| Message ID | 20240605134054.2626953-14-jmarchan@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace-cmd: fix misc issues found by static analysis | expand |
On Wed, 5 Jun 2024 15:40:28 +0200 "Jerome Marchand" <jmarchan@redhat.com> 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 <jmarchan@redhat.com> > --- > 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. > printf("Failed %s of %s\n", type, file); I'll skip this patch too. -- Steve
On 17/07/2024 22:51, Steven Rostedt wrote: > On Wed, 5 Jun 2024 15:40:28 +0200 > "Jerome Marchand" <jmarchan@redhat.com> 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 <jmarchan@redhat.com> >> --- >> 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
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: printf("Failed %s of %s\n", type, file);
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. Fixes a RESOURCE_LEAK error (CWE-772) Signed-off-by: Jerome Marchand <jmarchan@redhat.com> --- tracecmd/trace-record.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)