Re: [PATCH v2] grub-file: fix segmentation fault

[Andrei Borzenkov]

On Thu, Nov 17, 2016 at 10:08 PM, Daniel Kiper <address@hidden> wrote:
> On Tue, Apr 12, 2016 at 02:24:12PM +0800, Michael Chang wrote:
>> 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. This
>> patch tries to fix the problem.
>>
>>  gdb --args ./grub-file --is-x86_64-xen-domu 
>> /boot/vmlinux-4.1.12-1-default.gz
>>
>>  (gdb) bt
>>  #0  0x000000000047597e in grub_strlen (s=0x0) at 
>> ../grub-core/kern/misc.c:558
>>  #1  0x00000000004757e2 in grub_strdup (s=0x0) at 
>> ../grub-core/kern/misc.c:463
>>  #2  0x0000000000406418 in grub_elf_file (file=0x6dfb50, filename=0x0) at 
>> ../grub-core/kern/elf.c:89
>>  #3  0x00000000004043b3 in grub_xen_file (file=0x6dfb50) at 
>> ../grub-core/loader/i386/xen_file.c:29
>>  #4  0x0000000000403930 in grub_cmd_file (ctxt=0x7fffffffe120, argc=1, 
>> args=0x6dfa00) at ../grub-core/commands/file.c:425
>>  #5  0x000000000047e178 in grub_extcmd_dispatcher (cmd=0x6df730, argc=2, 
>> args=0x6ddfb0, script=0x0) at ../grub-core/commands/extcmd.c:54
>>  #6  0x000000000047e1d7 in grub_extcmd_dispatch (cmd=0x6df730, argc=2, 
>> args=0x6ddfb0) at ../grub-core/commands/extcmd.c:67
>>  #7  0x0000000000402945 in main (argc=3, argv=0x7fffffffe2e8) at 
>> ../util/grub-file.c:102
>>  (gdb) frame 3
>>  #3  0x00000000004043b3 in grub_xen_file (file=0x6dfb50) at 
>> ../grub-core/loader/i386/xen_file.c:29
>>  29        elf = grub_elf_file (file, file->name);
>>
>> v2: Keep the file->name in file io handle for filters as it would be fetched 
>> by
>> progress and used modules, according to the review by Andrei Borzenkov.
>
> Vladimir, Andrei, could you take care of it?
>

Hmm ... I must admit I am confused how we can get NULL here. Filters
are called after primary file->name is set and each filter copies
previous struct file, which means returned file will inherit pointer
to the same file name.

Now after quick glance there are filters (like bufio) that do not set
file->name to NULL on close which may lead to double free. Not sure if
it can be responsible for this. This needs fixing irrespective (as
long as we use current implementation).

Anyway, exactly because filters themselves do not free file->name this
patch means memory leak.

Michael, could you provide reproducer for it?