Re: [PATCH] progress: Check for NULL filename

[Andrei Borzenkov <arvidjaar@gmail.com>]

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
>> <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?
>

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 <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