Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak in clib_package_new #323

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Conversation

tyler92
Copy link
Contributor

@tyler92 tyler92 commented Dec 16, 2024

The leak was found by fuzzer:

=================================================================
==335809==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x55f752b5da5e in malloc (clib/myfuzzing/prefix/bin/fuzz_manifest+0x117a5e)
    #1 0x55f752bbfa4a in json_object_init clib/deps/parson/parson.c:291:42
    #2 0x55f752bbf8e9 in json_value_init_object clib/deps/parson/parson.c:1077:31
    #3 0x55f752bc839f in parse_object_value clib/deps/parson/parson.c:580:32
    #4 0x55f752bbd0e4 in parse_value clib/deps/parson/parson.c:561:20
    #5 0x55f752bc8a6e in parse_object_value clib/deps/parson/parson.c:600:21
    #6 0x55f752bbd0e4 in parse_value clib/deps/parson/parson.c:561:20
    #7 0x55f752bbc55a in json_parse_string clib/deps/parson/parson.c:910:12
    #8 0x55f752ba5b01 in clib_package_new clib/src/common/clib-package.c:468:16
    #9 0x55f752ba58b2 in clib_package_load_from_manifest clib/src/common/clib-package.c:259:9
    #10 0x55f752b9c9fa in LLVMFuzzerTestOneInput clib/test/fuzzing/fuzz_manifest.c:22:6
    #11 0x55f752aa8a70 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (clib/myfuzzing/prefix/bin/fuzz_manifest+0x62a70)
    #12 0x55f752aa81e5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (clib/myfuzzing/prefix/bin/fuzz_manifest+0x621e5)
    #13 0x55f752aa99c5 in fuzzer::Fuzzer::MutateAndTestOne() (clib/myfuzzing/prefix/bin/fuzz_manifest+0x639c5)
    #14 0x55f752aaa5d5 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (clib/myfuzzing/prefix/bin/fuzz_manifest+0x645d5)
    #15 0x55f752a9873b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (clib/myfuzzing/prefix/bin/fuzz_manifest+0x5273b)
    #16 0x55f752ac1772 in main (clib/myfuzzing/prefix/bin/fuzz_manifest+0x7b772)
    #17 0x7fe6cea2fd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: 32 byte(s) leaked in 1 allocation(s).

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    0 malloc
    1 json_object_init clib/deps/parson/parson.c:291:42
    2 json_value_init_object clib/deps/parson/parson.c:1077:31
    3 parse_object_value clib/deps/parson/parson.c:580:32
    4 parse_value clib/deps/parson/parson.c:561:20
    5 parse_object_value clib/deps/parson/parson.c:600:21
    6 parse_value clib/deps/parson/parson.c:561:20
    7 json_parse_string clib/deps/parson/parson.c:910:12
    8 clib_package_new clib/src/common/clib-package.c:468:16
    9 clib_package_load_from_manifest clib/src/common/clib-package.c:259:9
@jwerle jwerle merged commit 1d505c8 into clibs:master Dec 18, 2024
4 checks passed
@stephenmathieson
Copy link
Member

Thanks @tyler92!

Could you please share more information on how this was discovered? I would like to automate scanning the codebase for memory leaks and your input could be very valuable.

@tyler92
Copy link
Contributor Author

tyler92 commented Dec 19, 2024

Hi @stephenmathieson!

It was discovered by fuzzer integrated into OSS-Fuzz project. The main tool that helps to find such bugs (memory leaks, stack/heap buffer overflows, double free, etc) is Address sanitizer. So I just compiled it and launched it.

I would recommend two things for automation:

  • run unit tests with address sanitizer enabled
  • run fuzzers (btw, you can define new targets to cover the most interesting parts of the code) on each PR, e.g. with ClusterFuzzLite. I didn't do this integration by myself but I saw it many times for various open-source projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants