ASAN: heap-use-after-free when enabling H2 priorities

[zwoop]

Since we landed some new priority fixes on master, I enabled the priority feature on Docs for testing. I'm getting the following from ASAN.

Since we back ported that fix to 7.1.1 as well, this might need to be fixed there too.

=================================================================
==27424==ERROR: AddressSanitizer: heap-use-after-free on address 0x6020000224d8 at pc 0x0000008e3b35 bp 0x2aaaab1fe850 sp 0x2aaaab1fe840
READ of size 8 at 0x6020000224d8 thread T4 ([ET_NET 7])
    #0 0x8e3b34 in Http2DependencyTree<Http2Stream*>::_top(Http2DependencyTree<Http2Stream*>::Node*) /usr/local/src/trafficserver/proxy/http2/Http2DependencyTree.h:283
    #1 0x8e28c8 in Http2DependencyTree<Http2Stream*>::top() /usr/local/src/trafficserver/proxy/http2/Http2DependencyTree.h:296
    #2 0x8da605 in Http2ConnectionState::send_data_frames_depends_on_priority() /usr/local/src/trafficserver/proxy/http2/Http2ConnectionState.cc:1207
    #3 0x8d706f in Http2ConnectionState::main_event_handler(int, void*) /usr/local/src/trafficserver/proxy/http2/Http2ConnectionState.cc:880
    #4 0x6790ad in Continuation::handleEvent(int, void*) /usr/local/src/trafficserver/iocore/eventsystem/I_Continuation.h:153
    #5 0xb80eb0 in EThread::process_event(Event*, int) /usr/local/src/trafficserver/iocore/eventsystem/UnixEThread.cc:122
    #6 0xb81700 in EThread::execute() /usr/local/src/trafficserver/iocore/eventsystem/UnixEThread.cc:188
    #7 0xb7f249 in spawn_thread_internal /usr/local/src/trafficserver/iocore/eventsystem/Thread.cc:91
    #8 0x2b9314bfedc4 in start_thread (/lib64/libpthread.so.0+0x7dc4)
    #9 0x2b931561876c in clone (/lib64/libc.so.6+0xf776c)

0x6020000224d8 is located 8 bytes inside of 16-byte region [0x6020000224d0,0x6020000224e0)
freed by thread T4 ([ET_NET 7]) here:
    #0 0x6097c0 in operator delete(void*) (/opt/ats/bin/traffic_server+0x6097c0)
    #1 0x8cf589 in Http2DependencyTree<Http2Stream*>::Node::~Node() /usr/local/src/trafficserver/proxy/http2/Http2DependencyTree.h:67
    #2 0x8e2614 in Http2DependencyTree<Http2Stream*>::remove(Http2DependencyTree<Http2Stream*>::Node*) /usr/local/src/trafficserver/proxy/http2/Http2DependencyTree.h:217
    #3 0x8d95ef in Http2ConnectionState::delete_stream(Http2Stream*) /usr/local/src/trafficserver/proxy/http2/Http2ConnectionState.cc:1122
    #4 0x8daad2 in Http2ConnectionState::send_data_frames_depends_on_priority() /usr/local/src/trafficserver/proxy/http2/Http2ConnectionState.cc:1233
    #5 0x8d706f in Http2ConnectionState::main_event_handler(int, void*) /usr/local/src/trafficserver/proxy/http2/Http2ConnectionState.cc:880
    #6 0x6790ad in Continuation::handleEvent(int, void*) /usr/local/src/trafficserver/iocore/eventsystem/I_Continuation.h:153
    #7 0xb80eb0 in EThread::process_event(Event*, int) /usr/local/src/trafficserver/iocore/eventsystem/UnixEThread.cc:122
    #8 0xb81700 in EThread::execute() /usr/local/src/trafficserver/iocore/eventsystem/UnixEThread.cc:188
    #9 0xb7f249 in spawn_thread_internal /usr/local/src/trafficserver/iocore/eventsystem/Thread.cc:91
    #10 0x2b9314bfedc4 in start_thread (/lib64/libpthread.so.0+0x7dc4)

previously allocated by thread T4 ([ET_NET 7]) here:
    #0 0x609140 in operator new(unsigned long) (/opt/ats/bin/traffic_server+0x609140)
    #1 0x8e2ee1 in Http2DependencyTree<Http2Stream*>::Node::Node(unsigned int, unsigned int, unsigned int, Http2DependencyTree<Http2Stream*>::Node*, Http2Stream*) /usr/local/src/trafficserver/proxy/http2/Http2DependencyTree.h:61
    #2 0x8e1ce7 in Http2DependencyTree<Http2Stream*>::add(unsigned int, unsigned int, unsigned int, bool, Http2Stream*) /usr/local/src/trafficserver/proxy/http2/Http2DependencyTree.h:174
    #3 0x8d1bed in rcv_headers_frame /usr/local/src/trafficserver/proxy/http2/Http2ConnectionState.cc:291
    #4 0x8d7555 in Http2ConnectionState::main_event_handler(int, void*) /usr/local/src/trafficserver/proxy/http2/Http2ConnectionState.cc:898
    #5 0x6790ad in Continuation::handleEvent(int, void*) /usr/local/src/trafficserver/iocore/eventsystem/I_Continuation.h:153
    #6 0x8c7205 in send_connection_event /usr/local/src/trafficserver/proxy/http2/Http2ClientSession.cc:58
    #7 0x8ccbe5 in Http2ClientSession::do_complete_frame_read() /usr/local/src/trafficserver/proxy/http2/Http2ClientSession.cc:472
    #8 0x8cd10a in Http2ClientSession::state_process_frame_read(int, VIO*, bool) /usr/local/src/trafficserver/proxy/http2/Http2ClientSession.cc:509
    #9 0x8cb8da in Http2ClientSession::state_start_frame_read(int, void*) /usr/local/src/trafficserver/proxy/http2/Http2ClientSession.cc:402
    #10 0x8ca772 in Http2ClientSession::main_event_handler(int, void*) /usr/local/src/trafficserver/proxy/http2/Http2ClientSession.cc:307
    #11 0x6790ad in Continuation::handleEvent(int, void*) /usr/local/src/trafficserver/iocore/eventsystem/I_Continuation.h:153
    #12 0xb29651 in read_signal_and_update /usr/local/src/trafficserver/iocore/net/UnixNetVConnection.cc:124
    #13 0xb30857 in UnixNetVConnection::readSignalAndUpdate(int) /usr/local/src/trafficserver/iocore/net/UnixNetVConnection.cc:1079
    #14 0xaf22e4 in SSLNetVConnection::net_read_io(NetHandler*, EThread*) /usr/local/src/trafficserver/iocore/net/SSLNetVConnection.cc:597
    #15 0xb1573a in NetHandler::mainNetEvent(int, Event*) /usr/local/src/trafficserver/iocore/net/UnixNet.cc:497
    #16 0x6790ad in Continuation::handleEvent(int, void*) /usr/local/src/trafficserver/iocore/eventsystem/I_Continuation.h:153
    #17 0xb80eb0 in EThread::process_event(Event*, int) /usr/local/src/trafficserver/iocore/eventsystem/UnixEThread.cc:122
    #18 0xb81ed6 in EThread::execute() /usr/local/src/trafficserver/iocore/eventsystem/UnixEThread.cc:266
    #19 0xb7f249 in spawn_thread_internal /usr/local/src/trafficserver/iocore/eventsystem/Thread.cc:91
    #20 0x2b9314bfedc4 in start_thread (/lib64/libpthread.so.0+0x7dc4)

Thread T4 ([ET_NET 7]) created by T0 ([TS_MAIN]) here:
    #0 0x572758 in __interceptor_pthread_create (/opt/ats/bin/traffic_server+0x572758)
    #1 0xb7ebc0 in ink_thread_create ../../lib/ts/ink_thread.h:152
    #2 0xb7f4d2 in Thread::start(char const*, void*, unsigned long, std::function<void ()> const&) /usr/local/src/trafficserver/iocore/eventsystem/Thread.cc:111
    #3 0xb8643d in EventProcessor::spawn_event_threads(int, int, unsigned long) /usr/local/src/trafficserver/iocore/eventsystem/UnixEventProcessor.cc:336
    #4 0xb86a6d in EventProcessor::start(int, unsigned long) /usr/local/src/trafficserver/iocore/eventsystem/UnixEventProcessor.cc:385
    #5 0x6c0b4c in main /usr/local/src/trafficserver/proxy/Main.cc:1768
    #6 0x2b9315542b34 in __libc_start_main (/lib64/libc.so.6+0x21b34)

SUMMARY: AddressSanitizer: heap-use-after-free /usr/local/src/trafficserver/proxy/http2/Http2DependencyTree.h:283 in Http2DependencyTree<Http2Stream*>::_top(Http2DependencyTree<Http2Stream*>::Node*)
Shadow bytes around the buggy address:
  0x0c047fffc440: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fa
  0x0c047fffc450: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fd
  0x0c047fffc460: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
  0x0c047fffc470: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
  0x0c047fffc480: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
=>0x0c047fffc490: fa fa fd fd fa fa fd fd fa fa fd[fd]fa fa 00 00
  0x0c047fffc4a0: fa fa 00 00 fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fffc4b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fffc4c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fffc4d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fffc4e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==27424==ABORTING

[scw00]

@masaori335

Since I removed this assertion, it may remove wrong node if the node is not the top one ! I think it is the root cause for this case.

while (node->queue->empty() && node->parent != NULL) {
 -    ink_assert(node->parent->queue->top() == node->entry);		
 -		
      node->parent->queue->pop();		   
      node->queued = false;

[scw00]

we should use erase instead of pop

[masaori335]

@scw00 I agree, we should use erase.

[zwoop]

Makes sense. I assume we'd want this fix for 7.1.1 as well ?

[masaori335]

@zwoop Yes, because #2315 is backported to v7.1.1.

[zwoop]

Right.

[scw00]

HI @masaori335
Seeing this code, I confirm that will cause core dump. We need to fix the queue after reprioritize !

/** test for reprioritize with active node
*
*     root                  root                   root
*    /    \                /    \                 /    \
*   A      B   =======>   C      B   =======>    C      B
*           \            / 
*            C          A
*
*/
REGRESSION_TEST(Http2DependencyTree_reprioritize)(RegressionTest *t, int /* atype ATS_UNUSED */, int *pstatus)
{
  TestBox box(t, pstatus);
  box = REGRESSION_TEST_PASSED;

  Tree *tree = new Tree(100);
  string a("A"), b("B"), c("C");

  auto A = tree->add(0, 7, 70, false, &a);
  auto B = tree->add(0, 3, 10, false, &b);
  auto C = tree->add(3, 5, 30, false, &c);

  tree->activate(A);
  tree->activate(B);
  tree->activate(C);

  tree->reprioritize(A, 5, false);
  tree->deactivate(A, 0);

  tree->remove(A);

  box.check(tree->top()->t != nullptr, "data should not assertion");

  delete tree;
}

[masaori335]

@scw00 Nice catch! It looks like the tree still has B and C, but top() returns nullptr.

[masaori335]

@scw00 I got heap-use-after-free with ASan and your test case. The backtrace is similar. So this could be the same case which caused heap-use-after-free on Docs.
UPDATE: Sorry, I didn't pull latest master:p Now I see SEGV.