Re: [PATCH] progress: Check for NULL filename

[Andrei Borzenkov]

14.08.2015 00:04, dann frazier пишет:
> On Thu, Aug 13, 2015 at 10:52:19AM +0300, Andrei Borzenkov wrote:
> > On Wed, Aug 12, 2015 at 6:53 PM, dann frazier
> > <address@hidden> wrote:
> > > Avoid a NULL pointer dereference if the upper fs layer hasn't set the
> > > file->name field. Files opened through the grub_net_fs interface currently do
> > > not have this field set (though perhaps they should?).
> >
> > file->name is set in grub_file_open independently of any filesystem
> > used. How comes it becomes empty? Do you see it in current GIT
> > master?
>
> Yeah, I see it with current GIT master. Here's what I believe is happening.
>
> grub_file_open() calls the fs->open callback, *before* it initializes
> file->name. In the net_fs open callback (haven't checked others), it
> makes a copy of the file structure and instantiates a bufio file
> structure for it. It copies the bufio structure over the file
> structure that was passed in.
>
> Now we return to grub_file_open and set file->name in the (now bufio)
> file structure. But the original file structure backing the bufio
> still has a NULL name. When this bufio is read, it calls the read
> method on this backing file, which causes the progress hook to run and
> fall over.
>
> Perhaps the fix here is just to make grub_file_open set file->name
> before the fs_open callback?

Then grub_net_fs_open needs to duplicate file->name, otherwise it will 
result in double free of file->name (first by grub_file_close in 
grub_bufio_close and then by original grub_file_close).

And grub_file_open ignores NULL return from grub_strdup so we need check 
in progress anyway.

I wish we could simply use grub_buffile_open, but I do not see easy way.

Could you make combined patch?

> diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
> index 24da12b..4afa8c2 100644
> --- a/grub-core/kern/file.c
> +++ b/grub-core/kern/file.c
> @@ -99,10 +99,11 @@ grub_file_open (const char *name)
>          goto fail;
>       }
>
> +  file->name = grub_strdup (name);
> +
>     if ((file->fs->open) (file, file_name) != GRUB_ERR_NONE)
>       goto fail;
>
> -  file->name = grub_strdup (name);
>     grub_errno = GRUB_ERR_NONE;
>
>     for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled);
> @@ -126,6 +127,7 @@ grub_file_open (const char *name)
>
>     /* if (net) grub_net_close (net);  */
>
> +  grub_free (file->name);
>     grub_free (file);
>
>     grub_memcpy (grub_file_filters_enabled, grub_file_filters_all,
>
>
> > > Signed-off-by: dann frazier <address@hidden>
> > > ---
> > >   grub-core/lib/progress.c | 3 +--
> > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
> > > index 63a0767..2775554 100644
> > > --- a/grub-core/lib/progress.c
> > > +++ b/grub-core/lib/progress.c
> > > @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector 
> > > __attribute__ ((unused)),
> > >          percent = grub_divmod64 (100 * file->progress_offset,
> > >                                   file->size, 0);
> > >
> > > -      partial_file_name = grub_strrchr (file->name, '/');
> > > -      if (partial_file_name)
> > > +      if (file->name && (partial_file_name = grub_strrchr (file->name, '/')))
> > >          partial_file_name++;
> > >         else
> > >          partial_file_name = "";
> > >
> > >
> > > _______________________________________________
> > > Grub-devel mailing list
> > > address@hidden
> > > https://lists.gnu.org/mailman/listinfo/grub-devel