Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1688003005:
QUIC - instrumentation for testing null QuicSpdyStream. (Closed)
|
Created:
4 years, 10 months ago by ramant (doing other things) Modified:
4 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, ianswett, Buck Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionQUIC - instrumentation for testing null QuicSpdyStream.
BUG=585591
R=rch@chromium.org,eroman@chromium.org
Committed: https://crrev.com/100e39060a98afdc80f7a5140f6ba690746261b9
Cr-Commit-Position: refs/heads/master@{#375804}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Add more instrumentation #
Total comments: 15
Patch Set 3 : Fix comments for PatchSet 2 #
Total comments: 6
Patch Set 4 : Fix comments #Patch Set 5 : rebase #
Depends on Patchset: Messages
Total messages: 29 (12 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== QUIC - instrumentation for testing null QuicSpdyStream. BUG=585591 R=rch@chromium.org ========== to ========== QUIC - instrumentation for testing null QuicSpdyStream. BUG=585591 R=rch@chromium.org ==========
Hi Ryan, This crash is happening on Mac (1 crash a day) and in Dev, we will get more crashes. It doesn't seem to happen on windows. Would like to see if this points out that it is not some random memory corruption, but it is use after free. This code is similar to the instrumentation code eroman was using. Thanks, raman
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688003005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688003005/20001
eroman@chromium.org changed reviewers: + eroman@chromium.org
https://codereview.chromium.org/1688003005/diff/20001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/1688003005/diff/20001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:563: stream_->CrashIfInvalid(); Driveby comments: According to the crash data for that bug, this is going to blow up without telling us anything new since it will be a null-ptr de-ref (the crash data shows that the crashes in IsDoneReading are small offsets from 0 [1]) Based on the working theory in the bug, that stream_->Read() is executing code that ultimately NULL's stream_, I think a better instrumentation would be to see if QuicHttpStream::ResetStream() has been called. Since that will definitively confirm that code in Read() is causing a re-entrancy into this QuickHttpStream. How about before calling stream_->CrashIfInvalid() you assert that ResetStream() wasn't called? (Are there any other ways to cause stream_ to be NULL-ed?) Maybe you have a member variable 'int reset_stream_count_ = 0" which ResetStream() increments. If that gives us data, you can go a step further and add a base::Debug::StackTrace which captures the callstack that ResetStream() was called from. [1] https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.ptype%3D%...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688003005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688003005/40001
Tested this code by manually calling ResetStream() and by deleting |stream_| object explicitly. PTAL. https://codereview.chromium.org/1688003005/diff/20001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/1688003005/diff/20001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:563: stream_->CrashIfInvalid(); On 2016/02/11 02:16:33, eroman wrote: > Driveby comments: > > According to the crash data for that bug, this is going to blow up without > telling us anything new since it will be a null-ptr de-ref (the crash data shows > that the crashes in IsDoneReading are small offsets from 0 [1]) > > Based on the working theory in the bug, that stream_->Read() is executing code > that ultimately NULL's stream_, I think a better instrumentation would be to see > if QuicHttpStream::ResetStream() has been called. Since that will definitively > confirm that code in Read() is causing a re-entrancy into this QuickHttpStream. > > How about before calling stream_->CrashIfInvalid() you assert that ResetStream() > wasn't called? (Are there any other ways to cause stream_ to be NULL-ed?) > > Maybe you have a member variable 'int reset_stream_count_ = 0" which > ResetStream() increments. If that gives us data, you can go a step further and > add a base::Debug::StackTrace which captures the callstack that ResetStream() > was called from. > > [1] > https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.ptype%3D%... Acknowledged. Thanks Eric. Added more instrumentation. +1 to ResetStream() being called during Read(). Hopefully, new instrumentation will show the unusual conditions under which it could happen. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:563: CHECK(!read_is_called_); Assuming Read() is not a recursive call. If it could be called recursively, then will change this to a counter. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:573: stream_->CrashIfInvalid(); Added this CrashIfInvalid() in case some one stomped on the memory (we had seen only one or very few crashes on windows, but there were more crashes in linux platforms). This should never happen. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... File net/quic/quic_http_stream.h (right): https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.h:182: bool read_is_called_ = false; This is set to true to when we call Read(). Didn't want to rely on |stream_| is not corrupted (ie it has become a nullptr). Assumption: When Read() is called, ResetStream() wouldn't be called. |stream_| will not be deleted underneath us. Or |stream_| will not be corrupted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== QUIC - instrumentation for testing null QuicSpdyStream. BUG=585591 R=rch@chromium.org ========== to ========== QUIC - instrumentation for testing null QuicSpdyStream. BUG=585591 R=rch@chromium.org,eroman@chromium.org ==========
lgtm https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:563: CHECK(!read_is_called_); On 2016/02/11 19:45:46, ramant wrote: > Assuming Read() is not a recursive call. If it could be called recursively, then > will change this to a counter. I don't know enough about this code to say if this is right. Assuming there is no re-entrancy here, then looks good. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:567: int rv = stream_->Read(buf, buf_len); I suggest calling stream_->CrashIfInvalid() before calling Read() too, to assert that it wasn't bad upon entry. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:571: read_is_called_ = false; Might want to add CHECK(read_is_called_) here too, just for extra paranoia. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:587: #if defined(NDEBUG) I think you can just CHECK(false) here -- if this is reached, then will just crash later in ReadAvailableData() anyway right? https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... File net/quic/quic_http_stream.h (right): https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.h:182: bool read_is_called_ = false; On 2016/02/11 19:45:47, ramant wrote: > This is set to true to when we call Read(). Didn't want to rely on |stream_| is > not corrupted (ie it has become a nullptr). > > Assumption: When Read() is called, ResetStream() wouldn't be called. |stream_| > will not be deleted underneath us. Or |stream_| will not be corrupted. [optional] suggest naming this something closer to: "read_in_progress_" https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_spdy_stre... File net/quic/quic_spdy_stream.cc (right): https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_spdy_stre... net/quic/quic_spdy_stream.cc:57: void QuicSpdyStream::CrashIfInvalid() const { I believe you want to either copy stack_trace_ to the stack here, or remove stack_trace_ alltogether. stack_trace_ contains the deletion callstack, but based on what is being test I would expect the CHECK() in the dtor to get hit if we delete it during the Read(), meaning less useful to have it here.
Thanks Eric. Will wait for Ryan's comments (especially if ReadAvailableData() could be called recursively). My read of the code is, then there is an assumption that it won't happen. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:563: CHECK(!read_is_called_); On 2016/02/12 01:42:46, eroman wrote: > On 2016/02/11 19:45:46, ramant wrote: > > Assuming Read() is not a recursive call. If it could be called recursively, > then > > will change this to a counter. > > I don't know enough about this code to say if this is right. > Assuming there is no re-entrancy here, then looks good. Acknowledged. If there is a recursion and if that calls ResetStream() at line# 578, then that could be another reason why |stream_| could become nullptr. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:567: int rv = stream_->Read(buf, buf_len); On 2016/02/12 01:42:46, eroman wrote: > I suggest calling stream_->CrashIfInvalid() before calling Read() too, to assert > that it wasn't bad upon entry. Done. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:571: read_is_called_ = false; On 2016/02/12 01:42:46, eroman wrote: > Might want to add CHECK(read_is_called_) here too, just for extra paranoia. Done. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:587: #if defined(NDEBUG) On 2016/02/12 01:42:46, eroman wrote: > I think you can just CHECK(false) here -- if this is reached, then will just > crash later in ReadAvailableData() anyway right? Done. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... File net/quic/quic_http_stream.h (right): https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.h:182: bool read_is_called_ = false; On 2016/02/12 01:42:46, eroman wrote: > On 2016/02/11 19:45:47, ramant wrote: > > This is set to true to when we call Read(). Didn't want to rely on |stream_| > is > > not corrupted (ie it has become a nullptr). > > > > Assumption: When Read() is called, ResetStream() wouldn't be called. |stream_| > > will not be deleted underneath us. Or |stream_| will not be corrupted. > > [optional] suggest naming this something closer to: "read_in_progress_" Done. https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_spdy_stre... File net/quic/quic_spdy_stream.cc (right): https://codereview.chromium.org/1688003005/diff/40001/net/quic/quic_spdy_stre... net/quic/quic_spdy_stream.cc:57: void QuicSpdyStream::CrashIfInvalid() const { On 2016/02/12 01:42:46, eroman wrote: > I believe you want to either copy stack_trace_ to the stack here, or remove > stack_trace_ alltogether. > > stack_trace_ contains the deletion callstack, but based on what is being test I > would expect the CHECK() in the dtor to get hit if we delete it during the > Read(), meaning less useful to have it here. +1. Removed the stack_trace_. Done.
lgtm https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:563: stream_->CrashIfInvalid(); nit: move this up one line so it precedes other access on stream_ https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_http_stre... File net/quic/quic_http_stream.h (right): https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_http_stre... net/quic/quic_http_stream.h:182: bool read_is_progress_ = false; read_in_progress_ ? https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_spdy_stre... File net/quic/quic_spdy_stream.h (right): https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_spdy_stre... net/quic/quic_spdy_stream.h:156: void set_read_is_progress(bool value) { read_is_progress_ = value; } read_in_progress
Thanks Eric. Will not submit this until a new Dev branch is cut. https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:563: stream_->CrashIfInvalid(); On 2016/02/12 21:12:43, eroman wrote: > nit: move this up one line so it precedes other access on stream_ Good point. Thanks much. Done. https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_http_stre... File net/quic/quic_http_stream.h (right): https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_http_stre... net/quic/quic_http_stream.h:182: bool read_is_progress_ = false; On 2016/02/12 21:12:43, eroman wrote: > read_in_progress_ ? facepalm! Done. https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_spdy_stre... File net/quic/quic_spdy_stream.h (right): https://codereview.chromium.org/1688003005/diff/60001/net/quic/quic_spdy_stre... net/quic/quic_spdy_stream.h:156: void set_read_is_progress(bool value) { read_is_progress_ = value; } On 2016/02/12 21:12:43, eroman wrote: > read_in_progress Done.
lgtm
The CQ bit was checked by rtenneti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/1688003005/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688003005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688003005/100001
Message was sent while issue was closed.
Description was changed from ========== QUIC - instrumentation for testing null QuicSpdyStream. BUG=585591 R=rch@chromium.org,eroman@chromium.org ========== to ========== QUIC - instrumentation for testing null QuicSpdyStream. BUG=585591 R=rch@chromium.org,eroman@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== QUIC - instrumentation for testing null QuicSpdyStream. BUG=585591 R=rch@chromium.org,eroman@chromium.org ========== to ========== QUIC - instrumentation for testing null QuicSpdyStream. BUG=585591 R=rch@chromium.org,eroman@chromium.org Committed: https://crrev.com/100e39060a98afdc80f7a5140f6ba690746261b9 Cr-Commit-Position: refs/heads/master@{#375804} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/100e39060a98afdc80f7a5140f6ba690746261b9 Cr-Commit-Position: refs/heads/master@{#375804}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/1754813002/ by rtenneti@chromium.org. The reason for reverting is: We got the call stack that causes the crash. Reverting this change. Will add the check for nullptr and the unit tests. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
