| Message ID | 20240429163901.65239-1-cgoettsche@seltendoof.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [1/3] libselinux: free empty scandir(3) result | expand |
On Mon, Apr 29, 2024 at 11:39 AM Christian Göttsche <cgoettsche@seltendoof.de> wrote: > > From: Christian Göttsche <cgzones@googlemail.com> > > In case scandir(3) finds no entries still free the returned result to > avoid leaking it. > > Also do not override errno in case of a failure. > > Reported.by: Cppcheck > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > libselinux/src/booleans.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c > index c557df65..1ede8e2d 100644 > --- a/libselinux/src/booleans.c > +++ b/libselinux/src/booleans.c > @@ -53,7 +53,11 @@ int security_get_boolean_names(char ***names, int *len) > > snprintf(path, sizeof path, "%s%s", selinux_mnt, SELINUX_BOOL_DIR); > *len = scandir(path, &namelist, &filename_select, alphasort); > - if (*len <= 0) { > + if (*len < 0) { > + return -1; > + } > + if (*len == 0) { Changing this will allow scandir to fail and it continue, what's the point? > + free(namelist); > errno = ENOENT; > return -1; > } > -- > 2.43.0 > >
On Mon, 29 Apr 2024 at 22:35, William Roberts <bill.c.roberts@gmail.com> wrote: > > On Mon, Apr 29, 2024 at 11:39 AM Christian Göttsche > <cgoettsche@seltendoof.de> wrote: > > > > From: Christian Göttsche <cgzones@googlemail.com> > > > > In case scandir(3) finds no entries still free the returned result to > > avoid leaking it. > > > > Also do not override errno in case of a failure. > > > > Reported.by: Cppcheck > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > libselinux/src/booleans.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c > > index c557df65..1ede8e2d 100644 > > --- a/libselinux/src/booleans.c > > +++ b/libselinux/src/booleans.c > > @@ -53,7 +53,11 @@ int security_get_boolean_names(char ***names, int *len) > > > > snprintf(path, sizeof path, "%s%s", selinux_mnt, SELINUX_BOOL_DIR); > > *len = scandir(path, &namelist, &filename_select, alphasort); > > - if (*len <= 0) { > > + if (*len < 0) { > > + return -1; > > + } > > + if (*len == 0) { > > Changing this will allow scandir to fail and it continue, what's the point? What do you mean by "continue"? The function will still return -1 with errno set if scandir(3) returns <= 0, like it does currently. But currently if scandir() returns 0, we currently leak the pointer to the empty array. > > > + free(namelist); > > errno = ENOENT; > > return -1; > > } > > -- > > 2.43.0 > > > >
On Tue, Apr 30, 2024 at 9:35 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Mon, 29 Apr 2024 at 22:35, William Roberts <bill.c.roberts@gmail.com> wrote: > > > > On Mon, Apr 29, 2024 at 11:39 AM Christian Göttsche > > <cgoettsche@seltendoof.de> wrote: > > > > > > From: Christian Göttsche <cgzones@googlemail.com> > > > > > > In case scandir(3) finds no entries still free the returned result to > > > avoid leaking it. > > > > > > Also do not override errno in case of a failure. > > > > > > Reported.by: Cppcheck > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > --- > > > libselinux/src/booleans.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c > > > index c557df65..1ede8e2d 100644 > > > --- a/libselinux/src/booleans.c > > > +++ b/libselinux/src/booleans.c > > > @@ -53,7 +53,11 @@ int security_get_boolean_names(char ***names, int *len) > > > > > > snprintf(path, sizeof path, "%s%s", selinux_mnt, SELINUX_BOOL_DIR); > > > *len = scandir(path, &namelist, &filename_select, alphasort); > > > - if (*len <= 0) { > > > + if (*len < 0) { > > > + return -1; > > > + } > > > + if (*len == 0) { > > > > Changing this will allow scandir to fail and it continue, what's the point? > > What do you mean by "continue"? > The function will still return -1 with errno set if scandir(3) returns > <= 0, like it does currently. > But currently if scandir() returns 0, we currently leak the pointer to > the empty array. I completely misread that, my apologies. I just mocked scandir for all those paths with the leak sanitizer enabled, lgtm. > > > > > > + free(namelist); > > > errno = ENOENT; > > > return -1; > > > } > > > -- > > > 2.43.0 > > > > > >
diff --git a/libselinux/src/booleans.c b/libselinux/src/booleans.c index c557df65..1ede8e2d 100644 --- a/libselinux/src/booleans.c +++ b/libselinux/src/booleans.c @@ -53,7 +53,11 @@ int security_get_boolean_names(char ***names, int *len) snprintf(path, sizeof path, "%s%s", selinux_mnt, SELINUX_BOOL_DIR); *len = scandir(path, &namelist, &filename_select, alphasort); - if (*len <= 0) { + if (*len < 0) { + return -1; + } + if (*len == 0) { + free(namelist); errno = ENOENT; return -1; }