rust-bpf / rust-bcc

user-friendly rust bindings for the bpf compiler collection
MIT License
475 stars 54 forks source link

Pass cflags as a pointer of pointers #132

Closed javierhonduco closed 4 years ago

javierhonduco commented 4 years ago

In #130 the cflags array changed from being a pointer to a contiguous array of pointers to chars *mut *const c_char, to being a pointer to a list of CStrings, which cause invalid memory reads.

This can be easily reproduced when we pass more than 1 cflags:

$ target/debug/hi         
[1]    14517 segmentation fault (core dumped)  target/debug/hi

Stacktrace from GDB:

``` #0 0x00007ffff5a0f1c5 in __strlen_avx2 () from /lib64/libc.so.6 #1 0x00007ffff53b1f20 in clang::driver::Driver::ParseDriverMode(llvm::StringRef, llvm::ArrayRef) () from /lib64/libclangDriver.so.8 #2 0x00007ffff53c61fd in clang::driver::Driver::BuildCompilation(llvm::ArrayRef) () from /lib64/libclangDriver.so.8 #3 0x00007ffff5dedf94 in ebpf::ClangLoader::do_compile(std::unique_ptr >*, ebpf::TableStorage&, bool, std::vector > const&, std::vector > const&, std::__cxx11::basic_string, std::allocator > const&, std::unique_ptr > const&, std::__cxx11::basic_string, std::allocator > const&, ebpf::FuncSource&, std::__cxx11::basic_string, std::allocator >&, bool, std::__cxx11::basic_string, std::allocator > const&, std::map, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > >, std::less, std::allocator, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > > > > >&, std::map, std::allocator >, std::vector, std::allocator >, std::allocator, std::allocator > > >, std::less, std::allocator > >, std::allocator, std::allocator > const, std::vector, std::allocator >, std::allocator, std::allocator > > > > > >&) () from /lib64/libbcc.so.0 #4 0x00007ffff5df0837 in ebpf::ClangLoader::parse(std::unique_ptr >*, ebpf::TableStorage&, std::__cxx11::basic_string, std::allocator > const&, bool, char const**, int, std::__cxx11::basic_string, std::allocator > const&, ebpf::FuncSource&, std::__cxx11::basic_string, std::allocator >&, std::__cxx11::basic_string, std::allocator > const&, std::map, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > >, std::less, std::allocator, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > > > > >&, std::map, std::allocator >, std::vector, std::allocator >, std::allocator, std::allocator > > >, std::less, std::allocator > >, std::allocator, std::allocator > const, std::vector, std::allocator >, std::allocator, std::allocator > > > > > >&) () from /lib64/libbcc.so.0 #5 0x00007ffff5d75eb6 in ebpf::BPFModule::load_cfile(std::__cxx11::basic_string, std::allocator > const&, bool, char const**, int) () from /lib64/libbcc.so.0 #6 0x00007ffff5d7bfd2 in ebpf::BPFModule::load_string(std::__cxx11::basic_string, std::allocator > const&, char const**, int) () from /lib64/libbcc.so.0 #7 0x00007ffff5d74ce0 in bpf_module_create_c_from_string () from /lib64/libbcc.so.0 #8 0x000055555557b15a in bcc::core::BPFBuilder::build (self=...) at /home/javierhonduco/rust-bcc/src/core/mod.rs:202 #9 0x000055555555ec9f in hi::do_main (runnable=...) at src/main.rs:10 #10 0x000055555555ee70 in hi::main () at src/main.rs:29 ```

Valgrind report:

``` $ valgrind target/debug/hi ==14628== Memcheck, a memory error detector ==14628== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==14628== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info ==14628== Command: target/debug/hi ==14628== Hello, world! ==14628== Invalid read of size 1 ==14628== at 0x483BBE6: strlen (vg_replace_strmem.c:461) ==14628== by 0x734FF1F: clang::driver::Driver::ParseDriverMode(llvm::StringRef, llvm::ArrayRef) (in /usr/lib64/libclangDriver.so.8) ==14628== by 0x73641FC: clang::driver::Driver::BuildCompilation(llvm::ArrayRef) (in /usr/lib64/libclangDriver.so.8) ==14628== by 0x4B9AF93: ebpf::ClangLoader::do_compile(std::unique_ptr >*, ebpf::TableStorage&, bool, std::vector > const&, std::vector > const&, std::__cxx11::basic_string, std::allocator > const&, std::unique_ptr > const&, std::__cxx11::basic_string, std::allocator > const&, ebpf::FuncSource&, std::__cxx11::basic_string, std::allocator >&, bool, std::__cxx11::basic_string, std::allocator > const&, std::map, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > >, std::less, std::allocator, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > > > > >&, std::map, std::allocator >, std::vector, std::allocator >, std::allocator, std::allocator > > >, std::less, std::allocator > >, std::allocator, std::allocator > const, std::vector, std::allocator >, std::allocator, std::allocator > > > > > >&) (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x4B9D836: ebpf::ClangLoader::parse(std::unique_ptr >*, ebpf::TableStorage&, std::__cxx11::basic_string, std::allocator > const&, bool, char const**, int, std::__cxx11::basic_string, std::allocator > const&, ebpf::FuncSource&, std::__cxx11::basic_string, std::allocator >&, std::__cxx11::basic_string, std::allocator > const&, std::map, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > >, std::less, std::allocator, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > > > > >&, std::map, std::allocator >, std::vector, std::allocator >, std::allocator, std::allocator > > >, std::less, std::allocator > >, std::allocator, std::allocator > const, std::vector, std::allocator >, std::allocator, std::allocator > > > > > >&) (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x4B22EB5: ebpf::BPFModule::load_cfile(std::__cxx11::basic_string, std::allocator > const&, bool, char const**, int) (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x4B28FD1: ebpf::BPFModule::load_string(std::__cxx11::basic_string, std::allocator > const&, char const**, int) (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x4B21CDF: bpf_module_create_c_from_string (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x12F159: bcc::core::BPFBuilder::build (imp.rs:472) ==14628== by 0x112C9E: find_at (dfa.rs:615) ==14628== by 0x112C9E: find_at (exec.rs:590) ==14628== by 0x112C9E: hi::do_main (re_bytes.rs:593) ==14628== by 0x112E6F: find_at (dfa.rs:655) ==14628== by 0x112E6F: find_at (exec.rs:590) ==14628== by 0x112E6F: hi::main (re_bytes.rs:593) ==14628== by 0x1176FA: find_at (rt.rs:67) ==14628== by 0x1176FA: find_at (exec.rs:590) ==14628== by 0x1176FA: std::rt::lang_start::{{closure}} (re_bytes.rs:593) ==14628== Address 0x13 is not stack'd, malloc'd or (recently) free'd ==14628== ==14628== ==14628== Process terminating with default action of signal 11 (SIGSEGV): dumping core ==14628== Access not within mapped region at address 0x13 ==14628== at 0x483BBE6: strlen (vg_replace_strmem.c:461) ==14628== by 0x734FF1F: clang::driver::Driver::ParseDriverMode(llvm::StringRef, llvm::ArrayRef) (in /usr/lib64/libclangDriver.so.8) ==14628== by 0x73641FC: clang::driver::Driver::BuildCompilation(llvm::ArrayRef) (in /usr/lib64/libclangDriver.so.8) ==14628== by 0x4B9AF93: ebpf::ClangLoader::do_compile(std::unique_ptr >*, ebpf::TableStorage&, bool, std::vector > const&, std::vector > const&, std::__cxx11::basic_string, std::allocator > const&, std::unique_ptr > const&, std::__cxx11::basic_string, std::allocator > const&, ebpf::FuncSource&, std::__cxx11::basic_string, std::allocator >&, bool, std::__cxx11::basic_string, std::allocator > const&, std::map, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > >, std::less, std::allocator, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > > > > >&, std::map, std::allocator >, std::vector, std::allocator >, std::allocator, std::allocator > > >, std::less, std::allocator > >, std::allocator, std::allocator > const, std::vector, std::allocator >, std::allocator, std::allocator > > > > > >&) (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x4B9D836: ebpf::ClangLoader::parse(std::unique_ptr >*, ebpf::TableStorage&, std::__cxx11::basic_string, std::allocator > const&, bool, char const**, int, std::__cxx11::basic_string, std::allocator > const&, ebpf::FuncSource&, std::__cxx11::basic_string, std::allocator >&, std::__cxx11::basic_string, std::allocator > const&, std::map, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > >, std::less, std::allocator, std::allocator >, int, int, int, int, unsigned int, std::__cxx11::basic_string, std::allocator > > > > >&, std::map, std::allocator >, std::vector, std::allocator >, std::allocator, std::allocator > > >, std::less, std::allocator > >, std::allocator, std::allocator > const, std::vector, std::allocator >, std::allocator, std::allocator > > > > > >&) (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x4B22EB5: ebpf::BPFModule::load_cfile(std::__cxx11::basic_string, std::allocator > const&, bool, char const**, int) (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x4B28FD1: ebpf::BPFModule::load_string(std::__cxx11::basic_string, std::allocator > const&, char const**, int) (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x4B21CDF: bpf_module_create_c_from_string (in /usr/lib64/libbcc.so.0.12.0) ==14628== by 0x12F159: bcc::core::BPFBuilder::build (imp.rs:472) ==14628== by 0x112C9E: find_at (dfa.rs:615) ==14628== by 0x112C9E: find_at (exec.rs:590) ==14628== by 0x112C9E: hi::do_main (re_bytes.rs:593) ==14628== by 0x112E6F: find_at (dfa.rs:655) ==14628== by 0x112E6F: find_at (exec.rs:590) ==14628== by 0x112E6F: hi::main (re_bytes.rs:593) ==14628== by 0x1176FA: find_at (rt.rs:67) ==14628== by 0x1176FA: find_at (exec.rs:590) ==14628== by 0x1176FA: std::rt::lang_start::{{closure}} (re_bytes.rs:593) ==14628== If you believe this happened as a result of a stack ==14628== overflow in your program's main thread (unlikely but ==14628== possible), you can try to increase the size of the ==14628== main thread stack using the --main-stacksize= flag. ==14628== The main thread stack size used in this run was 8388608. ==14628== ==14628== HEAP SUMMARY: ==14628== in use at exit: 438,477 bytes in 3,001 blocks ==14628== total heap usage: 5,416 allocs, 2,415 frees, 762,012 bytes allocated ==14628== ==14628== LEAK SUMMARY: ==14628== definitely lost: 0 bytes in 0 blocks ==14628== indirectly lost: 0 bytes in 0 blocks ==14628== possibly lost: 352 bytes in 1 blocks ==14628== still reachable: 438,125 bytes in 3,000 blocks ==14628== suppressed: 0 bytes in 0 blocks ==14628== Rerun with --leak-check=full to see details of leaked memory ==14628== ==14628== For lists of detected and suppressed errors, rerun with: -s ==14628== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 0 from 0) [1] 14628 segmentation fault (core dumped) valgrind target/debug/hi ```

With this PR, valgrind no longer reports any invalid reads.

$ valgrind target/debug/hi
==16177== Memcheck, a memory error detector
[...]
==16177== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Thanks!

javierhonduco commented 4 years ago

Seems like there is some transient? error with the llvm 9 builds

rdelfin commented 4 years ago

Looks like those errors are in master

brayniac commented 4 years ago

Looks like those errors are in master

Yeah - will need to look into that separately. Won't block the open PRs.

brayniac commented 4 years ago

Good catch. Thanks for the PR.