Fix double-free in NewModuleFromBufferArgs

[javierhonduco]

In
0238ec3 the freeing of C strings was changed to use defers. This can cause a double-free, which in the best case it will produce a crash. The reason why this happens is that the memory address at defer time is captured for later execution. If KConfigFilePath is less than 3, it was being freed and set to NULL. Once the defer executes on function return, the same address we already freed will be passed again.

We observed this while upgrading libbpfgo in Parca Agent (parca-dev/parca-agent#1599).

Test Plan

Verified it's a double free with ASAN

=================================================================
==171270==ERROR: AddressSanitizer: attempting double-free on 0x602000000010 in thread T14:
    #0 0x4d6e68 in __interceptor_free.part.0 asan_malloc_linux.cpp.o
    #1 0x3004be2 in _cgo_38fdf0a0bedf_Cfunc_free (/home/javierhonduco/code/parca-agent/dist/parca-agent+0x3004be2) (BuildId: aebc1e250e9da366a49de9206c528fb67b730e0b)
    #2 0x58bac3 in runtime.asmcgocall.abi0 runtime/asm_amd64.s:848

0x602000000010 is located 0 bytes inside of 1-byte region [0x602000000010,0x602000000011)
freed by thread T14 here:
    #0 0x4d6e68 in __interceptor_free.part.0 asan_malloc_linux.cpp.o
    #1 0x3004be2 in _cgo_38fdf0a0bedf_Cfunc_free (/home/javierhonduco/code/parca-agent/dist/parca-agent+0x3004be2) (BuildId: aebc1e250e9da366a49de9206c528fb67b730e0b)
    #2 0x58bac3 in runtime.asmcgocall.abi0 runtime/asm_amd64.s:848

previously allocated by thread T14 here:
    #0 0x4d7e37 in __interceptor_malloc (/home/javierhonduco/code/parca-agent/dist/parca-agent+0x4d7e37) (BuildId: aebc1e250e9da366a49de9206c528fb67b730e0b)
    #1 0x2ff3ff2 in _cgo_38fdf0a0bedf_Cfunc__Cmalloc (/home/javierhonduco/code/parca-agent/dist/parca-agent+0x2ff3ff2) (BuildId: aebc1e250e9da366a49de9206c528fb67b730e0b)
    #2 0x58bac3 in runtime.asmcgocall.abi0 runtime/asm_amd64.s:848

And that there are no issues with this patch applied, both while running the Agent with and without ASAN as well as while running the cpu profiling integration tests which exercise this code path.

[rafaeldtinoco]

Awesome. Thanks for reporting this! Will merge shortly.

[javierhonduco]

Thanks, Rafael! Would you mind cutting a release? 🙏

[geyslan]

Awesome, @javierhonduco thanks for this PR.

Since github is having issues right now, I'm going to review this here.

Shouldn't we let the defer C.free(unsafe.Pointer(cKconfigPath)) keeping the allocated address in line 386, and remove the C.free from 394, since 395 cKconfigPath = nil won't change the address in the deferred free?

This can be freeing a null pointer when len(args.KConfigFilePath) <= 2, leaving the old cKconfigPath address allocated.

[javierhonduco]

Shouldn't we let the defer C.free(unsafe.Pointer(cKconfigPath)) keeping the allocated address in line 386, and remove the C.free from 394, since 395 cKconfigPath = nil won't change the address in the deferred free?

I think that should work too!

This can be freeing a null pointer when len(args.KConfigFilePath) <= 2, leaving the old cKconfigPath address allocated.

Good catch! I had a slightly different change before and when updating I forgot to check this case

[rafaeldtinoco]

@geyslan and @yanivagman should we cut a libbpfgo release with this fix, bump tracee to include it, and release tracee afterwards ? Cutting a release of libbpfgo for @javierhonduco seems a nice thing to do.

[geyslan]

@geyslan and @yanivagman should we cut a libbpfgo release with this fix, bump tracee to include it, and release tracee afterwards ? Cutting a release of libbpfgo for @javierhonduco seems a nice thing to do.

👍🏼 fix week.

[geyslan]

LGTM