doh: memory leak in conjunction with proxy use #4463
Comments
|
I got blocked by this when trying to get the fuzzers running again. Here is a smaller testcase, as displayed by (a modified version of) read_corpus.py: Here is the base64 encoded data: Any modification of the following makes it not reproduce:
I guess it is not a coincidence that 127.0.1.127 is also set in https://github.com/pauldreik/curl-fuzzer/blob/c602bc13788fa88b7f93933b4e996aa1045c9dfd/curl_fuzzer.cc#L161 Any modification of the following, still reproduces the leak:
Hopefully that can assist in explaining this. I tried to understand where to clean up the doh structure, but I failed. I guess all this is related to to some reuse, and the old doh data structure is not cleared before settting a new one. Or something along those lines. |
|
This still requires that I build your custom fuzzer version, right? |
Yes, unfortunately. |
|
I needed to patch the build script, but after that I've reproduced the report - with debug symbols. |
|
Good you could reproduce it! Sorry the build script did not work, could you share what was wrong so I can fix it? |
|
I need to pass on sanitize options to my linker as well so I had to do this patch (I'll submit a fix for the leak in a minute): diff --git a/intree_fuzzer/scripts/build.sh b/intree_fuzzer/scripts/build.sh
index 871986f..fba2e02 100755
--- a/intree_fuzzer/scripts/build.sh
+++ b/intree_fuzzer/scripts/build.sh
@@ -73,11 +73,11 @@ case $tcname in
;;
clang6-asan-ubsan)
export CC=clang-6.0 CXX=clang++-6.0 CFLAGS="-fsanitize=address,undefined" CXXFLAGS="-fsanitize=address,undefined"
;;
clang7-asan-ubsan)
- export CC=clang-7 CXX=clang++-7 CFLAGS="-fsanitize=address,undefined" CXXFLAGS="-fsanitize=address,undefined"
+ export CC=clang-7 CXX=clang++-7 CFLAGS="-fsanitize=address,undefined" CXXFLAGS="-fsanitize=address,undefined" LDFLAGS="-fsanitize=address,undefined -fno-sanitize-recover=undefined,integer"
;;
clang8-asan-ubsan)
export CC=clang-8 CXX=clang++-8 CFLAGS="-fsanitize=address,undefined" CXXFLAGS="-fsanitize=address,undefined"
;;
clang7-asan-ubsan-O3) |
If curl_multi_cleanup() is called without all DOH handles having been removed, make sure this is done or they will leak their memory. Reported-by: Paul Dreik Fixes #4463
|
Thanks for the fix! It fixes the leak, but unfortunately it introduces a use after free. Here is the use after free test case base64 encoded: Hmm your build script change looks obviously needed, how can the build have worked for me? I use Debian Buster, were you use Debian testing for this test? |
|
I use debian unstable |
... or risk DoH memory leaks. Reported-by: Paul Dreik Fixes #4463
|
I'm pretty sure that my fix this time is good and should be landed no matter what, but I'll still wait a little longer for your feedback to see if there's any further polish you can think of or detect to be needed. |
|
Thanks, I will give it a go as soon as I can (probably tonight). |
|
Yep, merging ffc7526 fixes it! |
|
Thanks! |
I am running a modified version of the existing TLV http fuzzer (available here: https://github.com/pauldreik/curl-fuzzer/tree/paul/localfuzz_public0/intree_fuzzer/src/networkfuzzers).
I have gotten a leak when using doh and setting proxy option CURLOPT_PROXY (here: https://github.com/pauldreik/curl-fuzzer/blob/c602bc13788fa88b7f93933b4e996aa1045c9dfd/curl_fuzzer_tlv.cc#L291)
The fuzzer sets the following options:
The fuzzer (server) replies with the following garbage (the snipped tail is probably irrelevant):
and I get a leak report at the bottom of the message.
This reproduces well (tested two different machines, several times).
To reproduce, build the fuzzers according to https://github.com/pauldreik/curl-fuzzer/tree/paul/localfuzz_public0/intree_fuzzer#building and run (the provoking file is base64 encoded in the bottom of this report)
The test case file (läcka):
The text was updated successfully, but these errors were encountered: