[PATCH v2 5/6] efi_loader: move path out of file_handle

[Gabriel D'Alimonte]

Hi Ilias,

Thanks for the review!

On Thu, Feb 20, 2025 at 2:55 AM Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Gabriel,
>
> On Mon, 17 Feb 2025 at 20:31, Gabriel Dalimonte
> <gabriel.dalimonte at gmail.com> wrote:
> >
> > In order to support renaming via SetInfo(), path must allow for longer
> > values than what was originally present when file_handle was allocated.
> >
> > Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte at gmail.com>
> > ---
> >
> > (no changes since v1)
> >
> >  lib/efi_loader/efi_file.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> > index 201fa5f8f3c..6b15c1f3d27 100644
> > --- a/lib/efi_loader/efi_file.c
> > +++ b/lib/efi_loader/efi_file.c
> > @@ -40,7 +40,7 @@ struct file_handle {
> >         struct fs_dir_stream *dirs;
> >         struct fs_dirent *dent;
> >
> > -       char path[0];
> > +       char *path;
> >  };
> >  #define to_fh(x) container_of(x, struct file_handle, base)
> >
> > @@ -178,6 +178,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
> >                 u64 attributes)
> >  {
> >         struct file_handle *fh;
> > +       char *path;
> >         char f0[MAX_UTF8_PER_UTF16] = {0};
> >         int plen = 0;
> >         int flen = 0;
> > @@ -194,11 +195,13 @@ static struct efi_file_handle *file_open(struct file_system *fs,
> >                 plen = strlen(parent->path) + 1;
> >         }
> >
> > +       fh = calloc(1, sizeof(*fh));
> >         /* +2 is for null and '/' */
> > -       fh = calloc(1, sizeof(*fh) + plen + (flen * MAX_UTF8_PER_UTF16) + 2);
> > -       if (!fh)
> > -               return NULL;
> > +       path = calloc(1, plen + (flen * MAX_UTF8_PER_UTF16) + 2);
>
> I assume the new allocation gets freed in patch #6 right?
> Is there an easy way to include this change in the current patch?
> Otherwise, we'll end up not freeing memory on a specific sha commit.
> I can live with it if the change is too tricky though.

The `path` variable is assigned to `fh->path`

>
> > +       if (!fh || !path)
> > +               goto error;
> >
> > +       fh->path = path;

here. I used the intermediate `path` variable to collapse calloc
failure handling
into a single conditional. If it is preferable to split the
conditional and eliminate
`path`, I can make that change.

> >         fh->open_mode = open_mode;
> >         fh->base = efi_file_handle_protocol;
> >         fh->fs = fs;
> > @@ -245,6 +248,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
> >         return &fh->base;
> >
> >  error:
> > +       free(fh->path);
> >         free(fh);
> >         return NULL;
> >  }
> > @@ -368,6 +372,7 @@ out:
> >  static efi_status_t file_close(struct file_handle *fh)
> >  {
> >         fs_closedir(fh->dirs);
> > +       free(fh->path);
> >         free(fh);
> >         return EFI_SUCCESS;
> >  }
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias

Thanks,
Gabriel