On Mon, Apr 11, 2016 at 12:28:35PM +0300, Andrei Borzenkov wrote:
> On Mon, Apr 11, 2016 at 7:00 AM, Michael Chang <mch...@suse.com> wrote:
> > On Sat, Apr 09, 2016 at 07:01:50AM +0300, Andrei Borzenkov wrote:
> >> 08.04.2016 09:43, Michael Chang пишет:
> >> > In grub_file_open the file handle returned by file filters has no
> >> > file->name
> >> > set which leads to segmentation fault later referenced by grub_elf_file.
> >> > We
> >> > move the file->name value assignment after file filters to make sure it
> >> > will be
> >> > set and returned.
> >> >
> >>
> >> This now makes filename unavailable to progress module (which gets the
> >> last grub_file in a chain) and it still does not cover corner case of
> >> failing grub_strdup in grub_file_open.
> >
> > I don't get why the filename would, in the other way round to this patch
> > trying
> > to fix, become unavailable to progress module? As far as I see the file
> > progress read hook in grub_file_read would use the file handle returned
> > from grub_file_open and do not hold another chaining of opened files ..
> >
>
> porogress module for disk IO relies on file hooks; what we have is
>
> if (!file->read_hook)
> {
> file->read_hook = grub_file_progress_hook;
> file->read_hook_data = file;
> file->progress_offset = file->offset;
> }
>
> Each filter is implemented as pseudo-fs with own ->read function. This
> function calls lower fs or filter as needed. So we basically have
>
> grub_file_read (filter1)
> filter1->read
> grub_file_read (filter2)
> filter2->read
> grub_file_read (real_file)
> real_file->read
> disk_read
Thanks, I modified the send v2 patch to keep the original file->name as io
handle for filters. It will now only assign file->name if it's emtpy in
returned handle from filters to make sure it's available to user.
>
> Each grub_file_read will re-set file hooks to progress on its
> argument, but only the very last disk_read will actually interpret
> them. And it will get as data pointer to real_file and will fetch file
> name from there.
>
> Note that network code explicitly calls progress and does not rely on
> above framework at all.
>
> Oh ... and it looks like if user explicitly set file hooks on top
> level filter, these hooks are not propagated at all. Which may or may
> not be intentional.
>
> This is a mess, and needs cleanup.
It is really into something that I do not consider or expect to fix, it is too
far from fixing the segmentation fault problem. If we forced into that field
instead of a trivial fix that's really sad to me.
Thanks,
Michael
>
> > About covering the grub_strdup failure, the patch didn't do because it's not
> > the cause for the segfault so leaving it as it is, if you think it
> > necessary we
> > can handle the error by returning null handle of course.
>
> I do not know if this is intentional. Vladimir? Personally I'd say
> that if we could not allocate several bytes for file name, we unlikely
> can continue to do anything useful.
>
> >>
> >> Fixing the former requires some redesign. But as long as we allow
> >> filename to remain empty in grub_file_open every user must explicitly
> >> check for it being NULL.
> >
> > For what reason the filename returned by grub_file_open would be empty and
> > how
> > to know it reasonable from the user ? Adding the check is fine, but still a
> > bug
> > to me a filename is provided during grub_file_open but get ditched in
> > returned
> > handle without a reason.
> >
> > Thanks,
> > Michael
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
