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

[Rong Tao]

On 12/6/24 09:56, Quentin Monnet wrote:
> 2024-12-06 09:11 UTC+0800 ~ Rong Tao <rtoax@xxxxxxxxxxx>
> > On 12/6/24 05:34, Andrii Nakryiko wrote:
> > > On Thu, Dec 5, 2024 at 4:22 AM Quentin Monnet <qmo@xxxxxxxxxx> wrote:
> > > > On 05/12/2024 12:09, Rong Tao wrote:
> > > > > From: Rong Tao <rongtao@xxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxx>
> > > > Tested-by: Quentin Monnet <qmo@xxxxxxxxxx>
> > > > Reviewed-by: Quentin Monnet <qmo@xxxxxxxxxx>
> > > 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