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

Jump to bottom

Fix crashes when http2 parent stream gets aborted. #1125

Closed
wants to merge 1 commit into from
Closed

Fix crashes when http2 parent stream gets aborted. #1125

wants to merge 1 commit into from

Conversation

Andersbakken
Copy link
Contributor

@Andersbakken 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

@mention-bot
Copy link

@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
Copy link
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
Copy link
Member

bagder commented Nov 28, 2016

Thanks!

vulpine pushed a commit to vulpine/curl that referenced this pull request Nov 30, 2016
@Andersbakken
http2: Fix crashes when parent stream gets aborted
9d6e922 
Closes curl#1125
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
crash HTTP/2
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Andersbakken @mention-bot @tatsuhiro-t @bagder