Potential double-free related with proxy

[rouault]

First, sorry as I can't reproduce the bug reliably. This is an attempt at simplifying something I've been observing with the https://github.com/OSGeo/gdal project that runs under ossfuzz, and uses curl master HEAD.

We have a (not yet public) report in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35798 about a double-free occurring in Curl_free_request_state():

==490==ERROR: AddressSanitizer: attempting double-free on 0x617000000780 in thread T0:
--
  | SCARINESS: 42 (double-free)
  | #0 0x4b8ea2 in free /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:127:3
  | #1 0x297babd in Curl_free_request_state gdal/curl/lib/url.c:2184:3
  | #2 0x2939fb1 in multi_done gdal/curl/lib/multi.c:697:3
  | #3 0x29472b5 in multi_handle_timeout gdal/curl/lib/multi.c:1592:11
  | #4 0x293e8de in multi_runsingle gdal/curl/lib/multi.c:1767:10
  | #5 0x293dd8a in curl_multi_perform gdal/curl/lib/multi.c:2559:14
  | #6 0x56678a in cpl::MultiPerform(void*, void*) gdal/gdal/port/cpl_vsil_curl.cpp:819:16
  | #7 0x567648 in cpl::VSICurlHandle::GetFileSizeOrHeaders(bool, bool) gdal/gdal/port/cpl_vsil_curl.cpp:965:5
  | #8 0x54aff5 in cpl::VSICurlHandle::GetFileSize(bool) gdal/gdal/port/cpl_vsil_curl_class.h:390:65
  | #9 0x56ca58 in cpl::VSICurlHandle::Exists(bool) gdal/gdal/port/cpl_vsil_curl.cpp:1358:9
  | #10 0x5861f8 in cpl::VSICurlFilesystemHandler::Stat(char const*, stat64*, int) gdal/gdal/port/cpl_vsil_curl.cpp:4163:19
  | #11 0x820dd2 in VSIArchiveFilesystemHandler::SplitFilename(char const*, CPLString&, int) gdal/gdal/port/cpl_vsil_abstract_archive.cpp:541:34
  | #12 0x5e8ee9 in VSIZipFilesystemHandler::Open(char const*, char const*, bool, char const* const*) gdal/gdal/port/cpl_vsil_gzip.cpp:2802:25
  | #13 0x52132a in VSIFOpenEx2L gdal/gdal/port/cpl_vsil.cpp:1768:22
  | #14 0x51dbde in VSIFOpenExL gdal/gdal/port/cpl_vsil.cpp:1713:12
  | #15 0x51dba5 in VSIFOpenL gdal/gdal/port/cpl_vsil.cpp:1194:12
  | #16 0x683aa4 in OGRSpatialReference::SetFromUserInput(char const*, char const* const*) gdal/gdal/ogr/ogrspatialreference.cpp:3743:27
  | #17 0x682895 in OGRSpatialReference::SetFromUserInput(char const*) gdal/gdal/ogr/ogrspatialreference.cpp:3494:12
  | #18 0x68539c in OSRSetFromUserInput gdal/gdal/ogr/ogrspatialreference.cpp:3807:29
  | #19 0x4ec19e in LLVMFuzzerTestOneInput gdal/gdal/fuzzers/osr_set_from_user_input_fuzzer.cpp:56:19
  | #20 0x4ec84b in main
  | #21 0x7f8c34d3d83f in __libc_start_main /build/glibc-e6zv40/glibc-2.23/csu/libc-start.c:291
  | #22 0x43e648 in _start
  |  
  | 0x617000000780 is located 0 bytes inside of 656-byte region [0x617000000780,0x617000000a10)
  | freed by thread T0 here:
  | #0 0x4b8ea2 in free /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:127:3
  | #1 0x297d76b in conn_free gdal/curl/lib/url.c:792:3
  | #2 0x297cdda in Curl_disconnect gdal/curl/lib/url.c:875:3
  | #3 0x2939da7 in multi_done gdal/curl/lib/multi.c:664:12
  | #4 0x29472b5 in multi_handle_timeout gdal/curl/lib/multi.c:1592:11
  | #5 0x293e8de in multi_runsingle gdal/curl/lib/multi.c:1767:10
  | #6 0x293dd8a in curl_multi_perform gdal/curl/lib/multi.c:2559:14
  | #7 0x56678a in cpl::MultiPerform(void*, void*) gdal/gdal/port/cpl_vsil_curl.cpp:819:16
  | #8 0x567648 in cpl::VSICurlHandle::GetFileSizeOrHeaders(bool, bool) gdal/gdal/port/cpl_vsil_curl.cpp:965:5
  | #9 0x54aff5 in cpl::VSICurlHandle::GetFileSize(bool) gdal/gdal/port/cpl_vsil_curl_class.h:390:65
  | #10 0x56ca58 in cpl::VSICurlHandle::Exists(bool) gdal/gdal/port/cpl_vsil_curl.cpp:1358:9
  | #11 0x5861f8 in cpl::VSICurlFilesystemHandler::Stat(char const*, stat64*, int) gdal/gdal/port/cpl_vsil_curl.cpp:4163:19
  | #12 0x820dd2 in VSIArchiveFilesystemHandler::SplitFilename(char const*, CPLString&, int) gdal/gdal/port/cpl_vsil_abstract_archive.cpp:541:34
  | #13 0x5e8ee9 in VSIZipFilesystemHandler::Open(char const*, char const*, bool, char const* const*) gdal/gdal/port/cpl_vsil_gzip.cpp:2802:25
  | #14 0x52132a in VSIFOpenEx2L gdal/gdal/port/cpl_vsil.cpp:1768:22
  | #15 0x51dbde in VSIFOpenExL gdal/gdal/port/cpl_vsil.cpp:1713:12
  | #16 0x51dba5 in VSIFOpenL gdal/gdal/port/cpl_vsil.cpp:1194:12
  | #17 0x683aa4 in OGRSpatialReference::SetFromUserInput(char const*, char const* const*) gdal/gdal/ogr/ogrspatialreference.cpp:3743:27
  | #18 0x682895 in OGRSpatialReference::SetFromUserInput(char const*) gdal/gdal/ogr/ogrspatialreference.cpp:3494:12
  | #19 0x68539c in OSRSetFromUserInput gdal/gdal/ogr/ogrspatialreference.cpp:3807:29
  | #20 0x4ec19e in LLVMFuzzerTestOneInput gdal/gdal/fuzzers/osr_set_from_user_input_fuzzer.cpp:56:19
  | #21 0x4ec84b in main
  |  
  | previously allocated by thread T0 here:
  | #0 0x4b92a2 in calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
  | #1 0x29ffcab in connect_init gdal/curl/lib/http_proxy.c:169:9
  | #2 0x29ff8b6 in Curl_proxyCONNECT gdal/curl/lib/http_proxy.c:982:14
  | #3 0x29ff562 in Curl_proxy_connect gdal/curl/lib/http_proxy.c:117:14
  | #4 0x29eb461 in Curl_http_connect gdal/curl/lib/http.c:1492:12
  | #5 0x293f065 in multi_runsingle gdal/curl/lib/multi.c:1925:16
  | #6 0x293dd8a in curl_multi_perform gdal/curl/lib/multi.c:2559:14
  | #7 0x56678a in cpl::MultiPerform(void*, void*) gdal/gdal/port/cpl_vsil_curl.cpp:819:16
  | #8 0x567648 in cpl::VSICurlHandle::GetFileSizeOrHeaders(bool, bool) gdal/gdal/port/cpl_vsil_curl.cpp:965:5
  | #9 0x54aff5 in cpl::VSICurlHandle::GetFileSize(bool) gdal/gdal/port/cpl_vsil_curl_class.h:390:65
  | #10 0x56ca58 in cpl::VSICurlHandle::Exists(bool) gdal/gdal/port/cpl_vsil_curl.cpp:1358:9
  | #11 0x5861f8 in cpl::VSICurlFilesystemHandler::Stat(char const*, stat64*, int) gdal/gdal/port/cpl_vsil_curl.cpp:4163:19
  | #12 0x820dd2 in VSIArchiveFilesystemHandler::SplitFilename(char const*, CPLString&, int) gdal/gdal/port/cpl_vsil_abstract_archive.cpp:541:34
  | #13 0x5e8ee9 in VSIZipFilesystemHandler::Open(char const*, char const*, bool, char const* const*) gdal/gdal/port/cpl_vsil_gzip.cpp:2802:25
  | #14 0x52132a in VSIFOpenEx2L gdal/gdal/port/cpl_vsil.cpp:1768:22
  | #15 0x51dbde in VSIFOpenExL gdal/gdal/port/cpl_vsil.cpp:1713:12
  | #16 0x51dba5 in VSIFOpenL gdal/gdal/port/cpl_vsil.cpp:1194:12
  | #17 0x683aa4 in OGRSpatialReference::SetFromUserInput(char const*, char const* const*) gdal/gdal/ogr/ogrspatialreference.cpp:3743:27
  | #18 0x682895 in OGRSpatialReference::SetFromUserInput(char const*) gdal/gdal/ogr/ogrspatialreference.cpp:3494:12
  | #19 0x68539c in OSRSetFromUserInput gdal/gdal/ogr/ogrspatialreference.cpp:3807:29
  | #20 0x4ec19e in LLVMFuzzerTestOneInput gdal/gdal/fuzzers/osr_set_from_user_input_fuzzer.cpp:56:19
  | #21 0x4ec84b in main
  |  

I cannot say for sure which curl version was used, but the bug was created on Sun, Jul 4, 2021, 12:38 AM, so this was a curl version from that day or the day before. Actually the stats show that the bug was randomly hit from July 3 to July 26, and there are no occurences from it anymore (but ossfuzz still mentions it as reliably reproducing...), so it might have been solved but my digging into curl history didn't spot anything obvious to me (was wondering about c27a70a which was committed on July 25 and is related to http_proxy ?)

A minimum reproducer would be something like the following, but as I said I didn't manage to reproduce when trying different recent curl commits:

#include <curl/curl.h>

int main()
{
    CURL* h = curl_easy_init();
    curl_easy_setopt(h, CURLOPT_URL, "DICT\x2E\xFF\xFF/" "\x2E" "dwf\xFF&proxy:*:514?\xFF\xFF\xFF");
    curl_easy_setopt(h, CURLOPT_PROXY, "*:514?\xFF\xFF\xFF");
    curl_easy_perform(h);
    return 0;
}

[bagder]

It sounds like you might've spotted #7236, fixed in 14a2ca8

[rouault]

Thanks for the hint, so my real test indeed included a timeout, at 1 second. I've managed to reproduce (randomly, but quite frequentl) a crash with a stack trace similar to the one of ossfuzz on my local machine by checking out the commit just before 14a2ca8 and by trying the proxy on port 80 (where I've a Apache running) rather than 514, and with a tinyish timeout. So I assume that there must be something particular on ossfuzz infrastructure with port 514. who knows....

with

#include <curl/curl.h>

int main()
{
    while(1)
    {
    CURL* h = curl_easy_init();
    curl_easy_setopt(h, CURLOPT_URL, "DICT\x2E\xFF\xFF/" "\x2E" "dwf\xFF&proxy:*:514?\xFF\xFF\xFF");
    curl_easy_setopt(h, CURLOPT_PROXY, "*:80?\xFF\xFF\xFF");
    curl_easy_setopt(h, CURLOPT_CONNECTTIMEOUT_MS, 1);
    curl_easy_perform(h);
    curl_easy_cleanup(h);
    }
    return 0;
}

and Valgrind shows

==530521== Invalid free() / delete / delete[] / realloc()
==530521==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==530521==    by 0x489AF3C: curl_dbg_free (memdebug.c:292)
==530521==    by 0x48D8911: Curl_free_request_state (url.c:2167)
==530521==    by 0x48A4491: multi_done (multi.c:697)
==530521==    by 0x48A62E1: multi_runsingle (multi.c:1743)
==530521==    by 0x48A7D81: curl_multi_perform (multi.c:2520)
==530521==    by 0x487272D: easy_transfer (easy.c:606)
==530521==    by 0x48729A7: easy_perform (easy.c:696)
==530521==    by 0x4872A12: curl_easy_perform (easy.c:715)
==530521==    by 0x10937D: main (in /home/even/curl/mytest)
==530521==  Address 0x5250ed0 is 0 bytes inside a block of size 696 free'd
==530521==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==530521==    by 0x489AF3C: curl_dbg_free (memdebug.c:292)
==530521==    by 0x48D59C2: conn_free (url.c:775)
==530521==    by 0x48D5C6F: Curl_disconnect (url.c:858)
==530521==    by 0x48A4280: multi_done (multi.c:664)
==530521==    by 0x48A62E1: multi_runsingle (multi.c:1743)
==530521==    by 0x48A7D81: curl_multi_perform (multi.c:2520)
==530521==    by 0x487272D: easy_transfer (easy.c:606)
==530521==    by 0x48729A7: easy_perform (easy.c:696)
==530521==    by 0x4872A12: curl_easy_perform (easy.c:715)
==530521==    by 0x10937D: main (in /home/even/curl/mytest)
==530521==  Block was alloc'd at
==530521==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==530521==    by 0x489AC93: curl_dbg_calloc (memdebug.c:172)
==530521==    by 0x4892C1C: connect_init (http_proxy.c:169)
==530521==    by 0x48941D5: Curl_proxyCONNECT (http_proxy.c:978)
==530521==    by 0x4892A2C: Curl_proxy_connect (http_proxy.c:117)
==530521==    by 0x488ADFF: Curl_http_connect (http.c:1489)
==530521==    by 0x48A680F: multi_runsingle (multi.c:1901)
==530521==    by 0x48A7D81: curl_multi_perform (multi.c:2520)
==530521==    by 0x487272D: easy_transfer (easy.c:606)
==530521==    by 0x48729A7: easy_perform (easy.c:696)
==530521==    by 0x4872A12: curl_easy_perform (easy.c:715)
==530521==    by 0x10937D: main (in /home/even/curl/mytest)

But running a git bisect session reveals that the first commit to fix the issue is actually commit 0842175 (which landed on June 24th 2021)

Author: Richard Whitehouse <richard.whitehouse@metaswitch.com>
Date:   Thu Nov 30 16:56:53 2017 +0000

    multi: alter transfer timeout ordering
    
    - Check whether a connection has succeded before checking whether it's
      timed out.
    
      This means if we've connected quickly, but subsequently been
      descheduled, we allow the connection to succeed. Note, if we timeout,
      but between checking the timeout, and connecting to the server the
      connection succeeds, we will allow it to go ahead. This is viewed as
      an acceptable trade off.
    
    - Add additional failf logging around failed connection attempts to
      propogate the cause up to the caller.
    
    Co-Authored-by: Martin Howarth
    Closes #7178

[bagder]

Either way, you're still saying that you cannot reproduce the problem with the current curl source code, right? So we can close this issue then?

[rouault]

yes, let's close that. It seems it is no longer present in current curl source code, and the fact that the ossfuzz tickets are still open is probably just an artifact on ossfuzz.

[bagder]

Thanks!