On Thu, 2023-03-23 at 09:36 -0700, Andrii Nakryiko wrote:
[...]
> >
> > The documentation says that `realloc(ptr, 0)` frees `ptr`.
> > So, I assume that issue is caused by handling of empty sections.
>
> yep, thanks for repro steps! It's a quite interesting behavior. There
> are two reallocs involved:
>
> First, dst->raw_data is NULL, dst_final_sz is 0, realloc succeeds and
> returns non-NULL pointer (which according to documentation can be
> freed with free()). All good.
>
> Second one, for second file, we have non-NULL dst->raw_data returned
> from previous realloc(), we pass it to realloc() with dst_final_sz
> still 0. But *NOW* we get NULL as a return (and original special
> pointer "helpfully" freed for us). This we handle as -ENOMEM and exit.
>
> Amazingly non-error-prone behavior, of course.
Yep, a surprising behavior.
Can't find any historical context on why this was the choice.
>
> > This is easy to test using object files produced by LLVM:
> >
> > $ touch empty
> > $ llvm-objcopy --add-section .foobar=empty linked_maps1.bpf.o
> > $ llvm-objcopy --add-section .foobar=empty linked_maps2.bpf.o
> > $ bpftool --debug gen object linked_maps.linked1.o linked_maps1.bpf.o linked_maps2.bpf.o
> > libbpf: linker: adding object file 'linked_maps1.bpf.o'...
> > libbpf: linker: adding object file 'linked_maps2.bpf.o'...
> > Error: failed to link 'linked_maps2.bpf.o': Cannot allocate memory (12)
> > free(): double free detected in tcache 2
> > Aborted (core dumped)
> >
> > The valgrind output also matches the one attached to the original email.
> > Something like below fixes it:
> >
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index d7069780984a..ff3833e55c55 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -1113,7 +1113,7 @@ static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src
> > /* no need to re-align final size */
> > dst_final_sz = dst_align_sz + src->shdr->sh_size;
> >
> > - if (src->shdr->sh_type != SHT_NOBITS) {
> > + if (dst_final_sz != 0 && src->shdr->sh_type != SHT_NOBITS) {
> > tmp = realloc(dst->raw_data, dst_final_sz);
> > if (!tmp)
>
> let's maybe document this quirk instead of preventing realloc() call:
>
> /* comment here explaining the quirks of realloc() API and it's
> inconsistent runtime behavior */
> if (!tmp && dst_final_sz > 0)
> return -ENOMEM;
Agree.
>
> Eduard, are you going to send a proper patch for this? Thanks!
Will do, need to figure out how to encode the test case within selftests.
Still, would be good if James can confirm that the issue is fixed on his side.
>
> > return -ENOMEM;
> >
> >
> > BPF selftests are passing for me with this change,
> > objcopy-based reproducer no longer reports error.
> > WDYT?
> >
> > James, could you please test this patch with bpf-gcc?
> > (you will have to re-compile libbpf and bpftool,
> > I had to separately do `make -C tools/bpf/bpftool`
> > before re-building selftests for some reason)
> >
> > Thanks,
> > Eduard
> >
[...]
