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 a double-free for curl_multi_remove_handle #1083

Conversation

Andersbakken
Copy link
Contributor

@Andersbakken Andersbakken commented Oct 20, 2016

In short the easy handle needs to be disconnected from its connection at
this point since the connection still is serving other easy handles.

In our app we can reliably reproduce a crash in our http2 stress test
that is fixed by this change. I can't easily reproduce the same test in
a small example.

This is the gdb/asan output:

==11785==ERROR: AddressSanitizer: heap-use-after-free on address 0xe9f4fb80 at pc 0x09f41f19 bp 0xf27be688 sp 0xf27be67c
READ of size 4 at 0xe9f4fb80 thread T13 (RESOURCE_HTTP)
    #0 0x9f41f18 in curl_multi_remove_handle /path/to/source/3rdparty/curl/lib/multi.c:666

0xe9f4fb80 is located 0 bytes inside of 1128-byte region [0xe9f4fb80,0xe9f4ffe8)
freed by thread T13 (RESOURCE_HTTP) here:
    #0 0xf7b1b5c2 in __interceptor_free /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_malloc_linux.cc:45
    #1 0x9f7862d in conn_free /path/to/source/3rdparty/curl/lib/url.c:2808
    #2 0x9f78c6a in Curl_disconnect /path/to/source/3rdparty/curl/lib/url.c:2876
    #3 0x9f41b09 in multi_done /path/to/source/3rdparty/curl/lib/multi.c:615
    #4 0x9f48017 in multi_runsingle /path/to/source/3rdparty/curl/lib/multi.c:1896
    #5 0x9f490f1 in curl_multi_perform /path/to/source/3rdparty/curl/lib/multi.c:2123
    #6 0x9c4443c in perform /path/to/source/src/net/resourcemanager/ResourceManagerCurlThread.cpp:854
    #7 0x9c445e0 in ...
    #8 0x9c4cf1d in ...
    #9 0xa2be6b5 in ...
    #10 0xf7aa5780 in asan_thread_start /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_interceptors.cc:226
    #11 0xf4d3a16d in __clone (/lib/i386-linux-gnu/libc.so.6+0xe716d)

previously allocated by thread T13 (RESOURCE_HTTP) here:
    #0 0xf7b1ba27 in __interceptor_calloc /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_malloc_linux.cc:70
    #1 0x9f7dfa6 in allocate_conn /path/to/source/3rdparty/curl/lib/url.c:3904
    #2 0x9f88ca0 in create_conn /path/to/source/3rdparty/curl/lib/url.c:5797
    #3 0x9f8c928 in Curl_connect /path/to/source/3rdparty/curl/lib/url.c:6438
    #4 0x9f45a8c in multi_runsingle /path/to/source/3rdparty/curl/lib/multi.c:1411
    #5 0x9f490f1 in curl_multi_perform /path/to/source/3rdparty/curl/lib/multi.c:2123
    #6 0x9c4443c in perform /path/to/source/src/net/resourcemanager/ResourceManagerCurlThread.cpp:854
    #7 0x9c445e0 in ...
    #8 0x9c4cf1d in ...
    #9 0xa2be6b5 in ...
    #10 0xf7aa5780 in asan_thread_start /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_interceptors.cc:226
    #11 0xf4d3a16d in __clone (/lib/i386-linux-gnu/libc.so.6+0xe716d)

SUMMARY: AddressSanitizer: heap-use-after-free /path/to/source/3rdparty/curl/lib/multi.c:666 in curl_multi_remove_handle
Shadow bytes around the buggy address:
  0x3d3e9f20: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f30: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f50: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa
  0x3d3e9f60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x3d3e9f70:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9fa0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9fb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9fc0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==11785==ABORTING

Thread 14 "RESOURCE_HTTP" received signal SIGABRT, Aborted.
[Switching to Thread 0xf27bfb40 (LWP 12324)]
0xf7fd8be9 in __kernel_vsyscall ()
(gdb) bt

@mention-bot
Copy link

@Andersbakken, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @linusnielsen and @yangtse to be potential reviewers.

@Andersbakken Andersbakken force-pushed the fix-curl_multi_remove_handle-double-free branch from a5b2d40 to b5ce636 Compare October 20, 2016 16:48
In short the easy handle needs to be disconnected from its connection at
this point since the connection still is serving other easy handles.

In our app we can reliably reproduce a crash in our http2 stress test
that is fixed by this change. I can't easily reproduce the same test in
a small example.

This is the gdb/asan output:

==11785==ERROR: AddressSanitizer: heap-use-after-free on address 0xe9f4fb80 at pc 0x09f41f19 bp 0xf27be688 sp 0xf27be67c
READ of size 4 at 0xe9f4fb80 thread T13 (RESOURCE_HTTP)
    #0 0x9f41f18 in curl_multi_remove_handle /path/to/source/3rdparty/curl/lib/multi.c:666

0xe9f4fb80 is located 0 bytes inside of 1128-byte region [0xe9f4fb80,0xe9f4ffe8)
freed by thread T13 (RESOURCE_HTTP) here:
    #0 0xf7b1b5c2 in __interceptor_free /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_malloc_linux.cc:45
    curl#1 0x9f7862d in conn_free /path/to/source/3rdparty/curl/lib/url.c:2808
    curl#2 0x9f78c6a in Curl_disconnect /path/to/source/3rdparty/curl/lib/url.c:2876
    curl#3 0x9f41b09 in multi_done /path/to/source/3rdparty/curl/lib/multi.c:615
    curl#4 0x9f48017 in multi_runsingle /path/to/source/3rdparty/curl/lib/multi.c:1896
    curl#5 0x9f490f1 in curl_multi_perform /path/to/source/3rdparty/curl/lib/multi.c:2123
    curl#6 0x9c4443c in perform /path/to/source/src/net/resourcemanager/ResourceManagerCurlThread.cpp:854
    curl#7 0x9c445e0 in ...
    curl#8 0x9c4cf1d in ...
    curl#9 0xa2be6b5 in ...
    curl#10 0xf7aa5780 in asan_thread_start /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_interceptors.cc:226
    curl#11 0xf4d3a16d in __clone (/lib/i386-linux-gnu/libc.so.6+0xe716d)

previously allocated by thread T13 (RESOURCE_HTTP) here:
    #0 0xf7b1ba27 in __interceptor_calloc /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_malloc_linux.cc:70
    curl#1 0x9f7dfa6 in allocate_conn /path/to/source/3rdparty/curl/lib/url.c:3904
    curl#2 0x9f88ca0 in create_conn /path/to/source/3rdparty/curl/lib/url.c:5797
    curl#3 0x9f8c928 in Curl_connect /path/to/source/3rdparty/curl/lib/url.c:6438
    curl#4 0x9f45a8c in multi_runsingle /path/to/source/3rdparty/curl/lib/multi.c:1411
    curl#5 0x9f490f1 in curl_multi_perform /path/to/source/3rdparty/curl/lib/multi.c:2123
    curl#6 0x9c4443c in perform /path/to/source/src/net/resourcemanager/ResourceManagerCurlThread.cpp:854
    curl#7 0x9c445e0 in ...
    curl#8 0x9c4cf1d in ...
    curl#9 0xa2be6b5 in ...
    curl#10 0xf7aa5780 in asan_thread_start /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_interceptors.cc:226
    curl#11 0xf4d3a16d in __clone (/lib/i386-linux-gnu/libc.so.6+0xe716d)

SUMMARY: AddressSanitizer: heap-use-after-free /path/to/source/3rdparty/curl/lib/multi.c:666 in curl_multi_remove_handle
Shadow bytes around the buggy address:
  0x3d3e9f20: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f30: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f50: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa
  0x3d3e9f60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x3d3e9f70:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9f90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9fa0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9fb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3d3e9fc0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==11785==ABORTING

Thread 14 "RESOURCE_HTTP" received signal SIGABRT, Aborted.
[Switching to Thread 0xf27bfb40 (LWP 12324)]
0xf7fd8be9 in __kernel_vsyscall ()
 (gdb) bt
 #0  0xf7fd8be9 in __kernel_vsyscall ()
 curl#1  0xf4c7ee89 in __GI_raise (sig=6) at ../sysdeps/unix/sysv/linux/raise.c:54
 curl#2  0xf4c803e7 in __GI_abort () at abort.c:89
 curl#3  0xf7b2ef2e in __sanitizer::Abort () at /opt/toolchain/src/gcc-6.2.0/libsanitizer/sanitizer_common/sanitizer_posix_libcdep.cc:122
 curl#4  0xf7b262fa in __sanitizer::Die () at /opt/toolchain/src/gcc-6.2.0/libsanitizer/sanitizer_common/sanitizer_common.cc:145
 curl#5  0xf7b21ab3 in __asan::ScopedInErrorReport::~ScopedInErrorReport (this=0xf27be171, __in_chrg=<optimized out>) at /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_report.cc:689
 curl#6  0xf7b214a5 in __asan::ReportGenericError (pc=166993689, bp=4068206216, sp=4068206204, addr=3925146496, is_write=false, access_size=4, exp=0, fatal=true) at /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_report.cc:1074
 curl#7  0xf7b21fce in __asan::__asan_report_load4 (addr=3925146496) at /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_rtl.cc:129
 curl#8  0x09f41f19 in curl_multi_remove_handle (multi=0xf3406080, data=0xde582400) at /path/to/source3rdparty/curl/lib/multi.c:666
 curl#9  0x09f6b277 in Curl_close (data=0xde582400) at /path/to/source3rdparty/curl/lib/url.c:415
 curl#10 0x09f3354e in curl_easy_cleanup (data=0xde582400) at /path/to/source3rdparty/curl/lib/easy.c:860
 curl#11 0x09c6de3f in ...
 curl#12 0x09c378c5 in ...
 curl#13 0x09c48133 in ...
 curl#14 0x09c4d092 in ...
 curl#15 0x0a2be6b6 in ...
 curl#16 0xf7aa5781 in asan_thread_start (arg=0xf2d22938) at /opt/toolchain/src/gcc-6.2.0/libsanitizer/asan/asan_interceptors.cc:226
 curl#17 0xf5de52b5 in start_thread (arg=0xf27bfb40) at pthread_create.c:333
 curl#18 0xf4d3a16e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:114
@Andersbakken Andersbakken force-pushed the fix-curl_multi_remove_handle-double-free branch from b5ce636 to 0bca5ce Compare October 20, 2016 16:56
@jay
Copy link
Member

jay commented Oct 20, 2016

Curl_disconnect will stop to leave open connections if there are other users and for that matter multi will stop as well. Is it possible you have used a handle simultaneously from more than one thread? Once an easy handle is added to a multi handle it is part of that multi handle and cannot be used from another thread while the multi is being used. In other words, thread 1 is using a multi which contains an easy then thread 2 cannot use either while that's happening

@bagder
Copy link
Member

bagder commented Oct 21, 2016

I agree with @jay that I can't see the flow in the code how this change saves you. But I also acknowledge that @Andersbakken is not a rookie here and trust his ability to test and see if things get fixed so that actually feels like a stronger motivator than my reading of the code. This is when using HTTP/2, isn't it @Andersbakken ?

I can't see any harm in applying this change, it should actually make it clearer that the current handle is done using the connection from that moment on. Do you see any problems with applying it @jay ?

@bagder bagder added the crash label Oct 21, 2016
@jay
Copy link
Member

jay commented Oct 21, 2016

I don't grok the code as it concerns this. My read of it was it's designed that way and compensated for elsewhere. But I don't know why it's done that way. When I see something like that I think don't-rock-the-boat.

I agree with you about the reporter but I think we should get more information. Perhaps there is an issue with their threading implementation, there have been quite a few weird crashes reported that usually are caused by some violation of thread safety guidelines.

@Andersbakken
Copy link
Contributor Author

Hi Guys

There's definitely no concurrent access to any curl handles. All calls to curl happen from the same thread. I can try to make an example that reproduces this. Note that I've only seen it happen with http2. My repro environment uses an nghttp2 server and sends a ton requests at it.

Another catalyst for this behavior is that quite a few of these requests time out due to setting a low CURLOPT_TIMEOUT_MS. I think maybe the code-path where a request fails even when the connection is fine might have an issue.

@bagder
Copy link
Member

bagder commented Oct 22, 2016

As I already wrote, I'm fine with merging this without a reproducible test case since the change is so small and harmless. It would of course be much better to have a test case for this so that we can make sure we don't reintroduce this problem again in a future when we edit the code... But based on @Andersbakken's words and experience, I will merge this as a start.

@bagder bagder closed this in 406506c Oct 22, 2016
@jay
Copy link
Member

jay commented Oct 23, 2016

Anders can you try your stress test on aec0c99, ref #1075, it may be related.

@Andersbakken
Copy link
Contributor Author

Hi Jay

It does seem to still happen with that patch. I'm working on putting
together a stand-alone example that reproduces it.

Anders

On Sat, Oct 22, 2016 at 11:27 PM, Jay Satiro notifications@github.com
wrote:

Anders can you try your stress test on aec0c99
aec0c99,
ref #1075 #1075, it may be related.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1083 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEdSqSLzivEpF1QJ6jGUedHtVbYVtTsks5q2v5jgaJpZM4KcWhf
.

@Andersbakken
Copy link
Contributor Author

Thanks. Much appreciated. I'm working on a stand-alone libcurl example that
reproduces it.

Anders

On Sat, Oct 22, 2016 at 7:06 AM, Daniel Stenberg notifications@github.com
wrote:

Closed #1083 #1083 via 406506c
406506c
.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1083 (comment), or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAEdSnHsLEL_-Qa01XgYboCnUhgHKEaPks5q2hhMgaJpZM4KcWhf
.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants