Fix crashes when http2 parent stream gets aborted. #1125

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@Andersbakken
Contributor
Andersbakken commented Nov 15, 2016 edited

I can reproduce a use-after-free situation with libcurl and http2 by doing the following:

send A
send B which depends on A, apply dependency
abort B using curl_multi_remove_handle and subsequently free it using curl_easy_cleanup

Since A knows nothing about B's dependency on it we can't clean this up.

The patch fixes this by keeping a list of all children.

I believe the reprioritization is correct but it should be checked. I couldn't clearly understand what is supposed to happen if you do the following:

B depends on A
C depends on A exclusively
D depends on A

My assumption is that this should lead to:

D
C
B
A

E.g. C would lose its exclusivity when someone's dependency is applied afterwards. I'm not 100 sure this is correct.

The patch also allows for unsetting dependencies.

This is the output from our app reproducing the problem:

0xded96400 is malloc'ed for url: https://some-server.somewhere:1234/files/data-9m?bbbbb
0xded96400 depends on 0xded82400 (https://some-server.somewhere:1234/files/data-9m?bbbbb depends on https://some-server.somewhere:1234/files/data-10m?aaaaa)
0xded82400 is free'd for url: https://some-server.somewhere:1234/files/data-10m?aaaaa
=================================================================
==20684==ERROR: AddressSanitizer: heap-use-after-free on address 0xded82540 at pc 0x0a295b30 bp 0xf20fe708 sp 0xf20fe6fc
READ of size 4 at 0xded82540 thread T13 (RESOURCE_HTTP)
    #0 0xa295b2f in h2_pri_spec .../3rdparty/curl/lib/http2.c:1310

0xded82540 is located 320 bytes inside of 34700-byte region [0xded82400,0xded8ab8c)
freed by thread T13 (RESOURCE_HTTP) here:
    #0 0xf72b95c2 in __interceptor_free .../gcc-6.2.0/libsanitizer/asan/asan_malloc_linux.cc:45
    #1 0xa23c2cc in Curl_close .../3rdparty/curl/lib/url.c:494
    #2 0xa2037ff in curl_easy_cleanup .../3rdparty/curl/lib/easy.c:860
    #3 0x9f37f2d in ...
    #4 0x9efffa8 in ...
    #5 0x9f0ba4b in ...
    #6 0x9f1594d in ...
    #7 0xa58dc35 in ...
    #8 0xf7243780 in asan_thread_start .../gcc-6.2.0/libsanitizer/asan/asan_interceptors.cc:226
    #9 0xf44deeed in __clone (/lib/i386-linux-gnu/libc.so.6+0xe6eed)

previously allocated by thread T13 (RESOURCE_HTTP) here:
    #0 0xf72b9a27 in __interceptor_calloc .../gcc-6.2.0/libsanitizer/asan/asan_malloc_linux.cc:70
    #1 0xa23d1c6 in Curl_open .../3rdparty/curl/lib/url.c:631
    #2 0xa203160 in curl_easy_init .../3rdparty/curl/lib/easy.c:386
    #3 0x9f23c57 in ...
    #4 0x9f09c95 in ...
    #5 0x9f1590c in ...
    #6 0xa58dc35 in ...
    #7 0xf7243780 in asan_thread_start .../gcc-6.2.0/libsanitizer/asan/asan_interceptors.cc:226
    #8 0xf44deeed in __clone (/lib/i386-linux-gnu/libc.so.6+0xe6eed)

SUMMARY: AddressSanitizer: heap-use-after-free .../3rdparty/curl/lib/http2.c:1310 in h2_pri_spec
Shadow bytes around the buggy address:
  0x3bdb0450: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3bdb0460: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3bdb0470: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3bdb0480: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3bdb0490: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x3bdb04a0: fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd
  0x3bdb04b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3bdb04c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3bdb04d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3bdb04e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x3bdb04f0: 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
==20684==ABORTING
SIGNAL [6] at address 0x50cc!
    #0 0xf72c1ec4 in __sanitizer_print_stack_trace .../gcc-6.2.0/libsanitizer/asan/asan_stack.cc:36
    #1 0x8a0e389 in gibbon_signalHandler .../main.cpp:369

@Andersbakken Andersbakken Fix crashes when http2 parent stream gets aborted.
e76b1fa
@mention-bot

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

@tatsuhiro-t
Contributor

B depends on A
C depends on A exclusively
D depends on A

This leads to the following dependency tree:

B -> C
C -> A
D -> A

And yes, C loses exclusive status against A.

@bagder bagder closed this in 421f740 Nov 28, 2016
@bagder
Member
bagder commented Nov 28, 2016

Thanks!

@vulpine vulpine added a commit to vulpine/curl that referenced this pull request Nov 30, 2016
@Andersbakken @vulpine Andersbakken + vulpine http2: Fix crashes when parent stream gets aborted
Closes #1125
9d6e922
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment