> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_multi.c 
> b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> new file mode 100644
> index 000000000000..52d3339b430a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> @@ -0,0 +1,256 @@

[ ... ]

> +static __u32 *get_ids(const char * const funcs[], int funcs_cnt, const char 
> *mod)
> +{
> +     struct btf *btf, *vmlinux_btf = NULL;
> +     __u32 nr, type_id, cnt = 0;
> +     void *root = NULL;
> +     __u32 *ids = NULL;
> +     int i, err = 0;
> +
> +     btf = btf__load_vmlinux_btf();
> +     if (!ASSERT_OK_PTR(btf, "btf__load_vmlinux_btf"))
> +             return NULL;
> +
> +     if (mod) {
> +             vmlinux_btf = btf;
> +             btf = btf__load_module_btf(mod, vmlinux_btf);
> +             if (!ASSERT_OK_PTR(btf, "btf__load_module_btf")) {
> +                     btf__free(vmlinux_btf);
                        ^^^^

Does this code double-free vmlinux_btf when btf__load_module_btf() fails?

When btf__load_module_btf(mod, vmlinux_btf) fails, the error path at
ASSERT_OK_PTR calls btf__free(vmlinux_btf) and then jumps to the out:
label:

> +                     goto out;
> +             }
> +     }

[ ... ]

> +out:
> +     tdestroy(root, tdestroy_free_nop);
> +     btf__free(vmlinux_btf);
        ^^^^

which calls btf__free(vmlinux_btf) again on the same pointer.

Multiple reviewers ([email protected] and [email protected]) raised
this concern in v5. The fix should either set vmlinux_btf=NULL after the
first free or return directly without going to the out label.

Reference: 
https://lore.kernel.org/bpf/1ab81cf709ce7d536a898695ffad30c15c8a6a7bf48dcb7271239952dd270...@mail.kernel.org/

> +     ids = calloc(funcs_cnt, sizeof(ids[0]));
> +     if (!ids)
> +             goto out;
> +
> +     /*
> +      * We sort function names by name and search them
> +      * below for each function.
> +      */
> +     for (i = 0; i < funcs_cnt; i++) {
> +             if (!tsearch(&funcs[i], &root, compare)) {
> +                     ASSERT_FAIL("tsearch failed");
                        ^^^^

When tsearch() fails, should err be set to -1 before the goto?

Currently when tsearch() fails, we jump to out, but err is still 0.
Because err is 0, the cleanup block below won't free ids and will
instead return the partially or fully zero-initialized array:

> +                     goto out;
> +             }
> +     }

[ ... ]

> +     if (err) {
> +             free(ids);
> +             ids = NULL;
> +     }

This can lead to incorrect test behavior. The fix should set err=-1
before the goto out statement.

Reference: 
https://lore.kernel.org/bpf/[email protected]/

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24598000047

Reply via email to