| Message ID | 20210427183755.2790654-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | samples/landlock: fix path_list memory leak | expand |
On Tue, Apr 27, 2021 at 11:38 AM <trix@redhat.com> wrote: > > From: Tom Rix <trix@redhat.com> > > Clang static analysis reports this error > > sandboxer.c:134:8: warning: Potential leak of memory > pointed to by 'path_list' > ret = 0; > ^ > path_list is allocated in parse_path() but never freed. > > Signed-off-by: Tom Rix <trix@redhat.com> > --- > samples/landlock/sandboxer.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c > index 7a15910d2171..4629d011ed61 100644 > --- a/samples/landlock/sandboxer.c > +++ b/samples/landlock/sandboxer.c > @@ -134,6 +134,8 @@ static int populate_ruleset( > ret = 0; > > out_free_name: > + if (path_list) > + free(path_list); I don't think the conditional is even necessary? By our first `goto out_free_name;`, `parse_path` has already been called/memory for `path_list` has already been allocated. `parse_path` doesn't check whether `malloc` has failed. > free(env_path_name); > return ret; > } > -- > 2.26.3 >
On 27/04/2021 21:13, Nick Desaulniers wrote: > On Tue, Apr 27, 2021 at 11:38 AM <trix@redhat.com> wrote: >> >> From: Tom Rix <trix@redhat.com> >> >> Clang static analysis reports this error >> >> sandboxer.c:134:8: warning: Potential leak of memory >> pointed to by 'path_list' >> ret = 0; >> ^ >> path_list is allocated in parse_path() but never freed. >> >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> samples/landlock/sandboxer.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c >> index 7a15910d2171..4629d011ed61 100644 >> --- a/samples/landlock/sandboxer.c >> +++ b/samples/landlock/sandboxer.c >> @@ -134,6 +134,8 @@ static int populate_ruleset( >> ret = 0; >> >> out_free_name: >> + if (path_list) >> + free(path_list); > > I don't think the conditional is even necessary? By our first `goto > out_free_name;`, `parse_path` has already been called/memory for > `path_list` has already been allocated. `parse_path` doesn't check > whether `malloc` has failed. Indeed, no need for the path_list check. In practice, this memory leak doesn't stay long because of the execve, but I missed this free anyway. Thanks! Reviewed-by: Mickaël Salaün <mic@linux.microsoft.com> > >> free(env_path_name); >> return ret; >> } >> -- >> 2.26.3 >> > >
On 4/28/21 2:58 AM, Mickaël Salaün wrote: > On 27/04/2021 21:13, Nick Desaulniers wrote: >> On Tue, Apr 27, 2021 at 11:38 AM <trix@redhat.com> wrote: >>> From: Tom Rix <trix@redhat.com> >>> >>> Clang static analysis reports this error >>> >>> sandboxer.c:134:8: warning: Potential leak of memory >>> pointed to by 'path_list' >>> ret = 0; >>> ^ >>> path_list is allocated in parse_path() but never freed. >>> >>> Signed-off-by: Tom Rix <trix@redhat.com> >>> --- >>> samples/landlock/sandboxer.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c >>> index 7a15910d2171..4629d011ed61 100644 >>> --- a/samples/landlock/sandboxer.c >>> +++ b/samples/landlock/sandboxer.c >>> @@ -134,6 +134,8 @@ static int populate_ruleset( >>> ret = 0; >>> >>> out_free_name: >>> + if (path_list) >>> + free(path_list); >> I don't think the conditional is even necessary? By our first `goto >> out_free_name;`, `parse_path` has already been called/memory for >> `path_list` has already been allocated. `parse_path` doesn't check >> whether `malloc` has failed. > Indeed, no need for the path_list check. In practice, this memory leak > doesn't stay long because of the execve, but I missed this free anyway. > Thanks! Ok, the general problem of not checking if malloc and friends succeeds is a different problem. So remove the check and keep the free ? Tom > Reviewed-by: Mickaël Salaün <mic@linux.microsoft.com> > >>> free(env_path_name); >>> return ret; >>> } >>> -- >>> 2.26.3 >>> >>
On 28/04/2021 17:36, Tom Rix wrote: > > On 4/28/21 2:58 AM, Mickaël Salaün wrote: >> On 27/04/2021 21:13, Nick Desaulniers wrote: >>> On Tue, Apr 27, 2021 at 11:38 AM <trix@redhat.com> wrote: >>>> From: Tom Rix <trix@redhat.com> >>>> >>>> Clang static analysis reports this error >>>> >>>> sandboxer.c:134:8: warning: Potential leak of memory >>>> pointed to by 'path_list' >>>> ret = 0; >>>> ^ >>>> path_list is allocated in parse_path() but never freed. >>>> >>>> Signed-off-by: Tom Rix <trix@redhat.com> >>>> --- >>>> samples/landlock/sandboxer.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/samples/landlock/sandboxer.c >>>> b/samples/landlock/sandboxer.c >>>> index 7a15910d2171..4629d011ed61 100644 >>>> --- a/samples/landlock/sandboxer.c >>>> +++ b/samples/landlock/sandboxer.c >>>> @@ -134,6 +134,8 @@ static int populate_ruleset( >>>> ret = 0; >>>> >>>> out_free_name: >>>> + if (path_list) >>>> + free(path_list); >>> I don't think the conditional is even necessary? By our first `goto >>> out_free_name;`, `parse_path` has already been called/memory for >>> `path_list` has already been allocated. `parse_path` doesn't check >>> whether `malloc` has failed. >> Indeed, no need for the path_list check. In practice, this memory leak >> doesn't stay long because of the execve, but I missed this free anyway. >> Thanks! > > Ok, the general problem of not checking if malloc and friends succeeds > is a different problem. > > So remove the check and keep the free ? Yes please. > > Tom > >> Reviewed-by: Mickaël Salaün <mic@linux.microsoft.com> >> >>>> free(env_path_name); >>>> return ret; >>>> } >>>> -- >>>> 2.26.3 >>>> >>> >
diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c index 7a15910d2171..4629d011ed61 100644 --- a/samples/landlock/sandboxer.c +++ b/samples/landlock/sandboxer.c @@ -134,6 +134,8 @@ static int populate_ruleset( ret = 0; out_free_name: + if (path_list) + free(path_list); free(env_path_name); return ret; }