Re: [PATCH bpf-next v3] bpftool: Fix gen object segfault

[Rong Tao <rtoax@foxmail.com>]

On 12/6/24 09:56, Quentin Monnet wrote:
> 2024-12-06 09:11 UTC+0800 ~ Rong Tao <rtoax@foxmail.com>
>> On 12/6/24 05:34, Andrii Nakryiko wrote:
>>> On Thu, Dec 5, 2024 at 4:22 AM Quentin Monnet <qmo@kernel.org> wrote:
>>>> On 05/12/2024 12:09, Rong Tao wrote:
>>>>> From: Rong Tao <rongtao@cestc.cn>
>>>>>
>>>>> If the input file and output file are the same, the input file is
>>>>> cleared
>>>>> due to opening, resulting in a NULL pointer access by libbpf.
>>>>>
>>>>>       $ bpftool gen object prog.o prog.o
>>>>>       libbpf: failed to get ELF header for prog.o: invalid `Elf' handle
>>>>>       Segmentation fault
>>>>>
>>>>>       (gdb) bt
>>>>>       #0  0x0000000000450285 in linker_append_elf_syms
>>>>> (linker=0x4feda0, obj=0x7fffffffe100) at linker.c:1296
>>>>>       #1  bpf_linker__add_file (linker=0x4feda0, filename=<optimized
>>>>> out>, opts=<optimized out>) at linker.c:453
>>>>>       #2  0x000000000040c235 in do_object ()
>>>>>       #3  0x00000000004021d7 in main ()
>>>>>       (gdb) frame 0
>>>>>       #0  0x0000000000450285 in linker_append_elf_syms
>>>>> (linker=0x4feda0, obj=0x7fffffffe100) at linker.c:1296
>>>>>       1296              Elf64_Sym *sym = symtab->data->d_buf;
>>>>>
>>>>> Signed-off-by: Rong Tao <rongtao@cestc.cn>
>>>> Tested-by: Quentin Monnet <qmo@kernel.org>
>>>> Reviewed-by: Quentin Monnet <qmo@kernel.org>
>>> Isn't this papering over a deeper underlying issue? Why do we get
>>> SIGSEGV inside the linker at all instead of just erroring out?
>>> Comparison based on file path isn't a reliable way to check if input
>>> and output are both the same file, so this fixes the most obvious
>>> case, but not the actual issue.
>> Thanks for your replay! The current scenario is similar to the following
>> code.
>> After a.txt is opened in read mode, it is opened in write mode again, which
>> causes the contents of a.txt file to be cleared, resulting in no data
>> being read,
>>
>>
>>      fpr = fopen("a.txt", "r");
>>      fpw = fopen("a.txt", "w");
>>
>>      /* fgets() will get nothing, It's not glibc's fault. */
>>      while (fgets(buff, sizeof(buff), fpr))
>>          printf("%s", buff);
>>
>>      fprintf(fpw, "....");
>>
>>      fclose(fpr);
>>      fclose(fpw);
>>
>> corresponding to the SEGV of bpftool. Perhaps we can add the following
>> warning
>>
>>      if (x == NULL) {
>>          fprintf(stderr, "Maybe the file was opened for writing after
>> opened for read\n");
>>          return -EINVAL;
>>      }
>>
>> Whether this warning can be added may depend on libelf's processing. I will
>> try to fix this SEGV in libbpf, hopefully it can be fixed.
> Thank you Rong, I'm not sure I followed your explanation (the above is
> not bpftool code, is it?), but I think we just addressed the issue in
> libbpf with:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=e10500b69c3f3378f3dcfc8c2fe4cdb74fc844f5
Thanks for this, i just try this patch, there is still one problem as 
follows:

    $ cd tools/bpf/bpftool
    $ make -j8
    $ ls -l prog.o
    -rw-r--r--. 1 rongtao rongtao 78408 Dec  6 10:18 prog.o
    $ ./bpftool gen object prog.o prog.o
    libbpf: failed to get ELF header for prog.o: invalid `Elf' handle
    Error: failed to link 'prog.o': Invalid argument (22)
    $ ls -l prog.o
    -rw-r--r--. 1 rongtao rongtao 0 Dec  6 10:18 prog.o

The input file is cleared (size=0), which is not what the user expected.

Thanks,
Rong Tao
>
> We can drop the patch with the check on the names (sorry!). As Andrii
> mentioned, it's not very reliable to compare filenames. It's true that
> users can truncate files if they pass the same input and output file,
> but then that's the case with many command-line tools if you don't use
> them properly.
>
> So, no action required. Feel free to test with the patch above, the
> segfault should not longer occur.
>
> Thanks,
> Quentin