| Message ID | 20241028124815.47262-10-sjg@chromium.org |
|---|---|
| State | Rejected, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: Add support for logging to a buffer | expand |
Hi Simon, On Mon, 28 Oct 2024 at 14:48, Simon Glass <sjg@chromium.org> wrote: > > Freeing a NULL pointer is an error in EFI, so check the pointer first, > before freeing it. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > lib/efi_loader/efi_bootmgr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > index a3aa2b8d1b9..431a38704e9 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -1180,7 +1180,8 @@ out: > free(opt[i].lo); > } > free(opt); > - efi_free_pool(handles); > + if (handles) > + efi_free_pool(handles); We don't need this, efi_free_pool() checks the pointer already. Thanks /Ilias > > if (ret == EFI_NOT_FOUND) > return EFI_SUCCESS; > -- > 2.43.0 >
Hi Ilias, On Tue, 29 Oct 2024 at 11:02, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Mon, 28 Oct 2024 at 14:48, Simon Glass <sjg@chromium.org> wrote: > > > > Freeing a NULL pointer is an error in EFI, so check the pointer first, > > before freeing it. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > lib/efi_loader/efi_bootmgr.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > > index a3aa2b8d1b9..431a38704e9 100644 > > --- a/lib/efi_loader/efi_bootmgr.c > > +++ b/lib/efi_loader/efi_bootmgr.c > > @@ -1180,7 +1180,8 @@ out: > > free(opt[i].lo); > > } > > free(opt); > > - efi_free_pool(handles); > > + if (handles) > > + efi_free_pool(handles); > > We don't need this, efi_free_pool() checks the pointer already. Yes, but it then returns an error (EFI_INVALID_PARAMETER), which gets logged, with this series. Regards, Simon
Am 29. Oktober 2024 16:45:29 MEZ schrieb Simon Glass <sjg@chromium.org>: >Hi Ilias, > >On Tue, 29 Oct 2024 at 11:02, Ilias Apalodimas ><ilias.apalodimas@linaro.org> wrote: >> >> Hi Simon, >> >> On Mon, 28 Oct 2024 at 14:48, Simon Glass <sjg@chromium.org> wrote: >> > >> > Freeing a NULL pointer is an error in EFI, so check the pointer first, >> > before freeing it. >> > >> > Signed-off-by: Simon Glass <sjg@chromium.org> >> > --- >> > >> > lib/efi_loader/efi_bootmgr.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c >> > index a3aa2b8d1b9..431a38704e9 100644 >> > --- a/lib/efi_loader/efi_bootmgr.c >> > +++ b/lib/efi_loader/efi_bootmgr.c >> > @@ -1180,7 +1180,8 @@ out: >> > free(opt[i].lo); >> > } >> > free(opt); >> > - efi_free_pool(handles); >> > + if (handles) >> > + efi_free_pool(handles); >> >> We don't need this, efi_free_pool() checks the pointer already. > >Yes, but it then returns an error (EFI_INVALID_PARAMETER), which gets >logged, with this series. So this is not a problem of the existing code but of your patch series which creates a superfluous log message. Best regards Heinrich > >Regards, >Simon
Hi Heinrich, On Tue, 29 Oct 2024 at 23:13, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 29. Oktober 2024 16:45:29 MEZ schrieb Simon Glass <sjg@chromium.org>: > >Hi Ilias, > > > >On Tue, 29 Oct 2024 at 11:02, Ilias Apalodimas > ><ilias.apalodimas@linaro.org> wrote: > >> > >> Hi Simon, > >> > >> On Mon, 28 Oct 2024 at 14:48, Simon Glass <sjg@chromium.org> wrote: > >> > > >> > Freeing a NULL pointer is an error in EFI, so check the pointer first, > >> > before freeing it. > >> > > >> > Signed-off-by: Simon Glass <sjg@chromium.org> > >> > --- > >> > > >> > lib/efi_loader/efi_bootmgr.c | 3 ++- > >> > 1 file changed, 2 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > >> > index a3aa2b8d1b9..431a38704e9 100644 > >> > --- a/lib/efi_loader/efi_bootmgr.c > >> > +++ b/lib/efi_loader/efi_bootmgr.c > >> > @@ -1180,7 +1180,8 @@ out: > >> > free(opt[i].lo); > >> > } > >> > free(opt); > >> > - efi_free_pool(handles); > >> > + if (handles) > >> > + efi_free_pool(handles); > >> > >> We don't need this, efi_free_pool() checks the pointer already. > > > >Yes, but it then returns an error (EFI_INVALID_PARAMETER), which gets > >logged, with this series. > > So this is not a problem of the existing code but of your patch series which creates a superfluous log message. Is it an error to free a zero pointer in EFI? Regards, Simon
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a3aa2b8d1b9..431a38704e9 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -1180,7 +1180,8 @@ out: free(opt[i].lo); } free(opt); - efi_free_pool(handles); + if (handles) + efi_free_pool(handles); if (ret == EFI_NOT_FOUND) return EFI_SUCCESS;
Freeing a NULL pointer is an error in EFI, so check the pointer first, before freeing it. Signed-off-by: Simon Glass <sjg@chromium.org> --- lib/efi_loader/efi_bootmgr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)