New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 754823 link

Starred by 1 user
Status: Fixed
Owner:
█ xunji...@chromium.org
Last visit > 30 days ago
Closed: Aug 2017
Cc:
rch@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

net::QuicChromiumClientSession::Handle::ReleaseStream crash

Project Member Reported by xunji...@chromium.org, Aug 11 2017 
Internal bug number 64595248


signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0
Stack frame #00 pc 00000000001e4a4c  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine net::StreamPrecedence<unsigned int>::spdy3_priority() const at /b/build/slave/cronet-arm_64/build/src/out/Release/../../net/spdy/core/spdy_protocol.h:338
Stack frame #01 pc 00000000001e81a4  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine net::PriorityWriteScheduler<unsigned int>::RegisterStream(unsigned int, net::StreamPrecedence<unsigned int> const&) at /b/build/slave/cronet-arm_64/build/src/out/Release/../../net/spdy/core/priority_write_scheduler.h:60
Stack frame #02 pc 00000000001abd40  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine net::QuicChromiumClientStream::Handle::SaveState() at /b/build/slave/cronet-arm_64/build/src/out/Release/../../net/quic/chromium/quic_chromium_client_stream.cc:356
Stack frame #03 pc 00000000001ad09c  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine net::QuicChromiumClientStream::CreateHandle() at /b/build/slave/cronet-arm_64/build/src/out/Release/../../net/quic/chromium/quic_chromium_client_stream.cc:552 (discriminator 2)
Stack frame #04 pc 00000000001a6e00  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine net::QuicChromiumClientSession::Handle::ReleaseStream() at /b/build/slave/cronet-arm_64/build/src/out/Release/../../net/quic/chromium/quic_chromium_client_session.cc:341 (discriminator 2)
Stack frame #05 pc 00000000001a27a4  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine net::BidirectionalStreamQuicImpl::OnStreamReady(int) at /b/build/slave/cronet-arm_64/build/src/out/Release/../../net/quic/chromium/bidirectional_stream_quic_impl.cc:248 (discriminator 2)
Stack frame #06 pc 00000000000608c4  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine base::Callback<void (), (base::internal::CopyMode)0, (base::internal::RepeatMode)0>::Run() && at /b/build/slave/cronet-arm_64/build/src/out/Release/../../base/callback.h:91 (discriminator 2)
Stack frame #07 pc 00000000000752b0  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine base::MessageLoop::RunTask(base::PendingTask*) at /b/build/slave/cronet-arm_64/build/src/out/Release/../../base/message_loop/message_loop.cc:422
Stack frame #08 pc 00000000000755e0  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) at /b/build/slave/cronet-arm_64/build/src/out/Release/../../base/message_loop/message_loop.cc:433
Stack frame #09 pc 0000000000075868  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine base::MessageLoop::DoWork() at /b/build/slave/cronet-arm_64/build/src/out/Release/../../base/message_loop/message_loop.cc:540 (discriminator 4)
Stack frame #10 pc 00000000000774b4  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) at /b/build/slave/cronet-arm_64/build/src/out/Release/../../base/message_loop/message_pump_libevent.cc:220
Stack frame #11 pc 0000000000088a04  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine base::RunLoop::Run() at /b/build/slave/cronet-arm_64/build/src/out/Release/../../base/run_loop.cc:111
Stack frame #12 pc 00000000000a64fc  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine base::Thread::ThreadMain() at /b/build/slave/cronet-arm_64/build/src/out/Release/../../base/threading/thread.cc:338
Stack frame #13 pc 00000000000a2098  /data/app/com.foo.apps/lib/arm64/libcronet.61.0.3163.13.so: Routine base::(anonymous namespace)::ThreadFunc(void*) at /b/build/slave/cronet-arm_64/build/src/out/Release/../../base/threading/platform_thread_posix.cc:71
Stack frame #14 pc 0000000000067f90  /system/lib64/libc.so (_ZL15__pthread_startPv+196)
Stack frame #15 pc 000000000001d980  /system/lib64/libc.so (__start_thread+16)


What happens is after BidiStream successfully requested a session handle and before requesting a stream handle, the underlying session is closed. When BidiStream calls Session::Handle::ReleaseStream(), we have a segmentation fault because ReleaseStream() is trying to dereference a null session.


void BidirectionalStreamQuicImpl::OnStreamReady(int rv) {
  ...
  stream_ = session_->ReleaseStream();  // <-- crashes here
  ...
}

Since a QuicChromiumClientSession can be gone at anytime, I think a solution might be to make Session::Handle::ReleaseStream() safe to call when the underlying session is dead. Alternatively we can change BidirectionalStreamQuicImpl::OnStreamReady() to check QuicChromiumClientSession::Handle::IsConnected() before calling ReleaseStream().

Comment 1 by xunji...@chromium.org, Aug 11 2017 

Ryan: which approach do you think is appropriate here? 
I think QuicHttpStream might encounter the same issue -- underlying Quic session is gone before Session::Handle::ReleaseStream() is called.

Comment 2 by rch@chromium.org, Aug 11 2017 

Ugh. Thanks for getting to the bottom of this. I agree with your diagnosis, and prefer you first suggestion of making Session::Handle::ReleaseStream() safe to call when the underlying session is dead. Though I guess we'll have to modify the callers of ReleaseStream in any case, because if we make ReleaseStream() safe, it'll probably have to return nullptr so the caller will need to check for that. 

Does that work for you? Care to do the needful?

Comment 3 by xunji...@chromium.org, Aug 14 2017

Owner: xunji...@chromium.org
Status: Started (was: Untriaged)

Comment 4 by xunji...@chromium.org, Aug 14 2017

Labels: M-62
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aeed1e8f0d7e6643ea4b0ab7e59108a7d9705fd9

commit aeed1e8f0d7e6643ea4b0ab7e59108a7d9705fd9
Author: Helen Li <xunjieli@chromium.org>
Date: Mon Aug 14 23:52:30 2017

Fix QuicChromiumClientSession:Handle::ReleaseStream crash

When a QuicChromiumClientSession is gone after the StreamRequest has
succeeded, QuicChromiumClientSession::Handle::ReleaseStream() will call
into the StreamRequest which owns an invalid QuicChromiumClientStream ptr.
This CL adds a nullptr session check and adds a regression test.

Bug:  754823 
Change-Id: I41a3b62a343953122e158fbc132429afff8ad81f
Reviewed-on: https://chromium-review.googlesource.com/613440
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494253}
[modify] https://crrev.com/aeed1e8f0d7e6643ea4b0ab7e59108a7d9705fd9/net/quic/chromium/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/aeed1e8f0d7e6643ea4b0ab7e59108a7d9705fd9/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/aeed1e8f0d7e6643ea4b0ab7e59108a7d9705fd9/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/aeed1e8f0d7e6643ea4b0ab7e59108a7d9705fd9/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/aeed1e8f0d7e6643ea4b0ab7e59108a7d9705fd9/net/quic/chromium/quic_http_stream.cc

Comment 6 by xunji...@chromium.org, Aug 15 2017

Status: Fixed (was: Started)
►

Sign in to add a comment