Re: [PATCH] progress: Check for NULL filename

[dann frazier <dann.frazier@canonical.com>]

On Thu, Aug 13, 2015 at 10:52:19AM +0300, Andrei Borzenkov wrote:
> On Wed, Aug 12, 2015 at 6:53 PM, dann frazier
> <dann.frazier@canonical.com> 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?

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 <dann.frazier@canonical.com>
> > ---
> >  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
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel