New issue
Advanced search Search tips

Issue 874466 link

Starred by 2 users

Issue metadata

Status: Verified
Owner: ----
Closed: Sep 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

CHECK failure: !(read_side_closed_ && write_side_closed_) in quic_stream.cc

Project Member Reported by ClusterFuzz, Aug 15

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5365254167724032

Fuzzer: libFuzzer_net_quic_stream_factory_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !(read_side_closed_ && write_side_closed_) in quic_stream.cc
  quic::QuicStream::OnStreamFrame
  quic::QuicSpdyStream::OnInitialHeadersComplete
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=581969:581979

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5365254167724032

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.
 
Project Member

Comment 1 by ClusterFuzz, Aug 15

Components: Internals>Network>QUIC
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, Aug 15

Cc: renjietang@chromium.org zhongyi@chromium.org
Labels: Test-Predator-Auto-CC
Automatically adding ccs based on suspected regression changelists:

Do not force a started reader to start reading when migrate to a probing socket. by zhongyi@chromium.org - https://chromium.googlesource.com/chromium/src/+/d3d5f5023f75dfda721f62fb40e5ee80e3ce16e3

Add experiment of switching the client to GOAWAY when path degrading is detected. by renjietang@chromium.org - https://chromium.googlesource.com/chromium/src/+/a5722ccf3852547206c6c95e84fc0a95b4376068

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label.
renjietang@/zhongyi@ : Could you please look into this issue.
Cc: kkaluri@chromium.org
Labels: M-70 CF-NeedsTriage
Unable to find actual suspect through code search and also observing no CL's under regression range, hence adding appropriate label and requesting someone from dev team to look in to this issue.

Thanks!
I will take a look later today. 
There are merely two changes in the regression range. 

-----------------
Do not force a started reader to start reading when migrate to a probing socket. by zhongyi@chromium.org - https://chromium.googlesource.com/chromium/src/+/d3d5f5023f75dfda721f62fb40e5ee80e3ce16e3

The code moves a read upfront. Only affects migration code, connection migration is not supported in fuzzer as NCN is not provided. 
-----------------
Add experiment of switching the client to GOAWAY when path degrading is detected. by renjietang@chromium.org - https://chromium.googlesource.com/chromium/src/+/a5722ccf3852547206c6c95e84fc0a95b4376068

In general marks session as going away, and doesn't change the read/write behavior. 
I think I have get a clue of this case now. This test failure is not related to any change in the suspected regression range. 

The crash stack trace looks as follows:
#3 0x7f171d0e01b8 quic::QuicSpdyStream::OnStreamHeaderList()
#4 0x7f171d0da552 quic::QuicSpdySession::OnStreamHeaderList()
#5 0x7f171d0dbbad quic::QuicSpdySession::OnHeaderList()
#6 0x7f171d0dd964 quic::QuicSpdySession::SpdyFramerVisitor::OnHeaderFrameEnd()
#7 0x7f171d1f73cc http2::Http2DecoderAdapter::CommonHpackFragmentEnd()
#8 0x7f171d1f6ff3 http2::Http2DecoderAdapter::OnHeadersEnd()
#9 0x7f171cffdda7 http2::HeadersPayloadDecoder::StartDecodingPayload()
#10 0x7f171cff471c http2::Http2FrameDecoder::StartDecodingHeadersPayload()
#11 0x7f171cff3df0 http2::Http2FrameDecoder::StartDecodingPayload()
#12 0x7f171cff3951 http2::Http2FrameDecoder::DecodeFrame()
#13 0x7f171d1f470e http2::Http2DecoderAdapter::ProcessInputFrame()
#14 0x7f171d1f4433 http2::Http2DecoderAdapter::ProcessInput()
#15 0x7f171d0da60a quic::QuicSpdySession::ProcessHeaderData()
#16 0x7f171d0cb4cd quic::QuicHeadersStream::OnDataAvailable()
#17 0x7f171d19546e quic::QuicStreamSequencer::OnStreamFrame()
#18 0x7f171d18b7fa quic::QuicStream::OnStreamFrame()
#19 0x7f171d16fdcf quic::QuicSession::OnStreamFrame()
#20 0x7f171ce8e8ab net::QuicChromiumClientSession::OnStreamFrame()
#21 0x7f171d10053d quic::QuicConnection::OnStreamFrame()
#22 0x7f171d149b55 quic::QuicFramer::ProcessFrameData()
#23 0x7f171d147051 quic::QuicFramer::ProcessDataPacket()
#24 0x7f171d144c8f quic::QuicFramer::ProcessPacket()
#25 0x7f171d106dce quic::QuicConnection::ProcessUdpPacket()
#26 0x7f171d172980 quic::QuicSession::ProcessUdpPacket()
#27 0x7f171cea1f47 net::QuicChromiumClientSession::OnPacket()
#28 0x7f171ceabcb6 net::QuicChromiumPacketReader::ProcessReadResult()
#29 0x7f171ceababf net::QuicChromiumPacketReader::OnReadComplete()

Cc: rch@chromium.org
By the time we call on the QuicSpdySession::OnStreamHeaderList, the steam still has the read side open and thus we're able to get the existing stream by calling GetSpdyDataStream. [1]

And we then call QuicSpdyStream::OnStreamHeaderList() [2]

void QuicSpdyStream::OnStreamHeaderList(bool fin,
                                        size_t frame_len,
                                        const QuicHeaderList& header_list) {
                                                      <-------- stream read side still open
  // The headers list avoid infinite buffering by clearing the headers list
  // if the current headers are too large. So if the list is empty here
  // then the headers list must have been too large, and the stream should
  // be reset.
  // TODO(rch): Use an explicit "headers too large" signal. An empty header list
  // might be acceptable if it corresponds to a trailing header frame.
  if (header_list.empty()) {
    OnHeadersTooLarge();                  <--------------- closes stream with QUIC_HEADERS_TOO_LARGE
    if (IsDoneReading()) {                <--------------- returns false, didn't early return
      return;
    }
  }
  if (!headers_decompressed_) {
    OnInitialHeadersComplete(fin, frame_len, header_list);   <---------- blow up 
  } else {
    OnTrailingHeadersComplete(fin, frame_len, header_list);
  }
}


[1]https://cs.chromium.org/chromium/src/net/third_party/quic/core/http/quic_spdy_session.cc?type=cs&g=0&l=367
[2] https://cs.chromium.org/chromium/src/net/third_party/quic/core/http/quic_spdy_stream.cc?type=cs&sq=package:chromium&g=0&l=152
Good catch. I wonder if we should just skip the IsDoneReading() check there. Do you know if any tests fail with that configuration?
Labels: -Pri-1 Pri-2
Downgrading to P2 since this is not a regression but a corner case triggered by the change to the QuicStreamFactoryFuzzer. 

Yeah, I think we should do early return regardless of the return value of IsDoneReading() when the header is too large. 

There's no test coverage for this case, as the test will blow up otherwise. 
Project Member

Comment 11 by ClusterFuzz, Sep 27

ClusterFuzz has detected this issue as fixed in range 594520:594522.

Detailed report: https://clusterfuzz.com/testcase?key=5365254167724032

Fuzzer: libFuzzer_net_quic_stream_factory_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !(read_side_closed_ && write_side_closed_) in quic_stream.cc
  quic::QuicStream::OnStreamFrame
  quic::QuicSpdyStream::OnInitialHeadersComplete
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=581969:581979
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=594520:594522

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5365254167724032

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 12 by ClusterFuzz, Sep 27

Labels: ClusterFuzz-Verified
Status: Verified (was: Untriaged)
ClusterFuzz testcase 5365254167724032 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment