Add locking to evhttp_connection_connect. #219
Conversation
|
In order to make evhttp threadsafe, there should be locks associated with evhttp objects, right? Using the base lock here seems like an abstraction layer violation. |
|
Yeah, as mentioned in the issue, this is kind of a "minimum proof of concept", and it might be a good deal of changes still required to make it good. I decided to send it in, just to see what you thought about making evhttp thread-safe... I agree that using the lock in the base is a bit lame, but I'm also not entirely sure what's the approach used by libevent for locking... "One big lock" per |
|
So, can we close this? |
|
I think this fix helps and could be merged in. It doesn't fix everything, though, and in the end, when trying to add HTTPS client-side support, it ended up not being possible with only libevent, we had to switch to libevhtp (and many hacks), which was better, but still very difficult. |
|
@pphaneuf could you try to do next please #217 (comment) and see does this fix your case? And I agree with @nmathewson it is not correct to use |
|
That other fix doesn't seem to address this particular crash, no? More related to DNS? Not sure how to use the bufferevent lock, which I guess could be a slightly better one to use... I'm not using the HTTP client code in libevent at all in our project now, but the short test program in #217 was quite reliable in reproducing it, within a few runs, at worst. |
|
On Wed, Aug 19, 2015 at 06:07:14AM -0700, Pierre Phaneuf wrote:
I tried your program from #217, but the only thing that I got was with
|
|
Must be fixed by https://github.com/azat/libevent/tree/dns-related-fixes-v4, i.e. Can you test this? Thanks, |
If you use bufferevent_socket_connect_hostname() to resolve, then ipv4 answer
can be returned before ipv6 scheduled and if you will destroy bufferevent after
ipv4 answer will come then ipv6 will trigger UAF:
$ a.out
=================================================================
==29733==ERROR: AddressSanitizer: heap-use-after-free on address 0x60200000ef50 at pc 0x0000004b7aef bp 0x7fffffffd940 sp 0x7fffffffd0f8
READ of size 2 at 0x60200000ef50 thread T0
#0 0x4b7aee in __interceptor_index (/src/oss/libevent/libevent-github/.invest/217-evhttp-threaded/a.out+0x4b7aee)
#1 0x5060eb in string_num_dots /src/oss/libevent/libevent-github/.cmake-debug/../evdns.c:2739
#2 0x5078df in search_request_new /src/oss/libevent/libevent-github/.cmake-debug/../evdns.c:3214
libevent#3 0x506afd in evdns_base_resolve_ipv6 /src/oss/libevent/libevent-github/.cmake-debug/../evdns.c:2935
libevent#4 0x50aa94 in evdns_getaddrinfo /src/oss/libevent/libevent-github/.cmake-debug/../evdns.c:4719
libevent#5 0x51de4f in evutil_getaddrinfo_async_ /src/oss/libevent/libevent-github/.cmake-debug/../evutil.c:1567
libevent#6 0x4fe023 in bufferevent_socket_connect_hostname /src/oss/libevent/libevent-github/.cmake-debug/../bufferevent_sock.c:519
libevent#7 0x524f54 in evhttp_connection_connect_ /src/oss/libevent/libevent-github/.cmake-debug/../http.c:2493
libevent#8 0x525156 in evhttp_make_request /src/oss/libevent/libevent-github/.cmake-debug/../http.c:2548
libevent#9 0x52d373 in main (/src/oss/libevent/libevent-github/.invest/217-evhttp-threaded/a.out+0x52d373)
libevent#10 0x7ffff6849b44 in __libc_start_main /tmp/buildd/glibc-2.19/csu/libc-start.c:287
libevent#11 0x445806 in _start (/src/oss/libevent/libevent-github/.invest/217-evhttp-threaded/a.out+0x445806)
0x60200000ef50 is located 0 bytes inside of 15-byte region [0x60200000ef50,0x60200000ef5f)
freed by thread T1 here:
#0 0x4cc4f2 in __interceptor_free (/src/oss/libevent/libevent-github/.invest/217-evhttp-threaded/a.out+0x4cc4f2)
#1 0x5141c1 in event_mm_free_ /src/oss/libevent/libevent-github/.cmake-debug/../event.c:3512
#2 0x522402 in evhttp_connection_free /src/oss/libevent/libevent-github/.cmake-debug/../http.c:1206
libevent#3 0x52cc5f in connection_closer (/src/oss/libevent/libevent-github/.invest/217-evhttp-threaded/a.out+0x52cc5f)
libevent#4 0x50e80e in event_process_active_single_queue /src/oss/libevent/libevent-github/.cmake-debug/../event.c:1642
libevent#5 0x50ed57 in event_process_active /src/oss/libevent/libevent-github/.cmake-debug/../event.c:1734
libevent#6 0x50f458 in event_base_loop /src/oss/libevent/libevent-github/.cmake-debug/../event.c:1957
libevent#7 0x50eddf in event_base_dispatch /src/oss/libevent/libevent-github/.cmake-debug/../event.c:1768
libevent#8 0x52d075 in event_dispatch_thread (/src/oss/libevent/libevent-github/.invest/217-evhttp-threaded/a.out+0x52d075)
libevent#9 0x7ffff74fc0a3 in start_thread /tmp/buildd/glibc-2.19/nptl/pthread_create.c:309
Fixes: libevent#217
Closes: libevent#222
Closes: libevent#219
Gist: https://gist.github.com/azat/92cbb34232ac02d7972b (from libevent#217 but thread-safe)
See issue #217 for more background. Not sure that this really "fix it all", just cutting off one crash...