eroman@, could you please help to find an owner?

Ryan, can you find an owner for this?

Specifically this looks like a double-free of ReliableQuicStream, because both the QuicSession and QuicStreamFactory::OnSessionClosed() think they own it.

Specific sequence of events in this crash:

(1) QuicStreamFactory::OnSessionClosed() deletes its |session_|. [1]

(2) The destructors for |session_| run -- it is of type QuicChromiumClientSession, so runs super class dtors in sequence ~QuicClientSessionBase -> ~QuicSpdySession -> ~QuicSession.

(3) When executing the ~QuicSession the pointers in |closed_stream_| are freed. [2]

(4) One of the pointers in |closed_stream_| is presumably the same QuicChromiumClientSession() that OnSessionClosed() is currently deleting. Hence we get a double-free, which in this report labels as a type mismatch (because the vtables are being modified as the various dtors run). B00M!

[1]  https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_stream_factory.cc&sq=package:chromium&type=cs&l=1188

[2] https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_session.cc&sq=package:chromium&type=cs&l=60

Nice catch! jri: can you take a look at this?

On Thu, Apr 21, 2016 at 10:04 AM, eroman@chromium.org via Monorail <monorail@chromium.org> wrote:
>
>
> Comment #4 on  issue 605474  by eroman@chromium.org: Bad-cast to net::QuicSpdySession from net::QuicSession;quic_spdy_stream.cc:41:3
> https://bugs.chromium.org/p/chromium/issues/detail?id=605474#c4
>
> Specifically this looks like a double-free of ReliableQuicStream, because both the QuicSession and QuicStreamFactory::OnSessionClosed() think they own it.
>
> Specific sequence of events in this crash:
>
> (1) QuicStreamFactory::OnSessionClosed() deletes its |session_|. [1]
>
> (2) The destructors for |session_| run -- it is of type QuicChromiumClientSession, so runs super class dtors in sequence ~QuicClientSessionBase -> ~QuicSpdySession -> ~QuicSession.
>
> (3) When executing the ~QuicSession the pointers in |closed_stream_| are freed. [2]
>
> (4) One of the pointers in |closed_stream_| is presumably the same QuicChromiumClientSession() that OnSessionClosed() is currently deleting. Hence we get a double-free, which in this report labels as a type mismatch (because the vtables are being modified as the various dtors run). B00M!


I'm not sure I follow this completely. The object deleted in (1) is a session, but the object deleted in (4) is a stream (not a session). So that doesn't sound like a double delete. Of course, clearly there's a bug here, but I don't yet see the sequence of events that leads to it.

RE comment #8:

Sorry yes, I got tripped up by all of the class hierarchies, and missed a step at the end!


So in step (4) when freeing |closed_stream_|, it deletes a (ReliableQuicStream), which is of type QuicSpdyStream. That object has a (non-owned) pointer back to the *session* which is currently in the process of being destroyed:

QuicSpdyStream::~QuicSpdyStream() {
  spdy_session_->UnregisterStreamPriority(id());
}

So in this last step calling UnregisterStreamPriority() is effectively a use-after-free (not a double free as I earlier said).

Does that seem more plausible?

That sounds totally right... and rather disturbing :(

Thanks.

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

commit 4ee9d76953f16811186942971822cf12d48db7ae
Author: jri <jri@chromium.org>
Date: Fri Apr 29 02:04:35 2016

Fixes use-after-free bug in QUIC.

QuicSession destructor ends up calling the QuicSpdyStream destructor, which in turn tries to access a QuicSession method via a non-owning pointer back to the session. This CL arranges for the non-owning pointer to be set to null before QuicSpdyStream destructor is called.

Merge Internal CL: 120973813

Creates a MockQuicSession for use in ReliableQuicStreamTest, which currently incorrectly uses a MockQuicSpdySession.

The test class being fixed in this CL uses a MockQuicSpdySession, which is a QuicSpdySession, with TestStreams, which are ReliableQuicStreams. Bad things can happen since QuicSpdySession seems to freely assume that its streams are QuicSpdyStreams. Bad things do happen, but they inexplicably seem to show up only in Chromium tests.

Merge Internal CL: 120989117

BUG= 605474 

Review-Url: https://codereview.chromium.org/1932653002
Cr-Commit-Position: refs/heads/master@{#390575}

[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/quic_session_test.cc
[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/quic_spdy_session.cc
[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/quic_spdy_stream.cc
[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/quic_spdy_stream.h
[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/reliable_quic_stream_test.cc
[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/test_tools/quic_session_peer.cc
[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/test_tools/quic_session_peer.h
[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/test_tools/quic_test_utils.cc
[modify] https://crrev.com/4ee9d76953f16811186942971822cf12d48db7ae/net/quic/test_tools/quic_test_utils.h

mmoroz: this should be fixed. can you verify using the fuzzer?

Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz

jri@, thanks! Actually ClusterFuzz should check it automatically.

Your change meets the bar and is auto-approved for M51 (branch: 2704)

Please merge your change to M51 branch 2704 ASAP (before 5:00 PM PST today). So we can take it for this week beta release tomorrow. Thank you.

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

commit 72c4e825fc4c65a8898ac3b3bf53092aba9787e3
Author: jri <jri@chromium.org>
Date: Wed May 11 21:43:46 2016

Fixes use-after-free bug in QUIC.

QuicSession destructor ends up calling the QuicSpdyStream destructor, which in turn tries to access a QuicSession method via a non-owning pointer back to the session. This CL arranges for the non-owning pointer to be set to null before QuicSpdyStream destructor is called.

Merge Internal CL: 120973813

Creates a MockQuicSession for use in ReliableQuicStreamTest, which currently incorrectly uses a MockQuicSpdySession.

The test class being fixed in this CL uses a MockQuicSpdySession, which is a QuicSpdySession, with TestStreams, which are ReliableQuicStreams. Bad things can happen since QuicSpdySession seems to freely assume that its streams are QuicSpdyStreams. Bad things do happen, but they inexplicably seem to show up only in Chromium tests.

Merge Internal CL: 120989117

BUG= 605474 
NOTRY=true
NOPRESUBMIT=true
Review-Url: https://codereview.chromium.org/1932653002
Cr-Commit-Position: refs/heads/master@{#390575}
(cherry picked from commit 4ee9d76953f16811186942971822cf12d48db7ae)

Review-Url: https://codereview.chromium.org/1975483003
Cr-Commit-Position: refs/branch-heads/2704@{#505}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/quic_session_test.cc
[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/quic_spdy_session.cc
[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/quic_spdy_stream.cc
[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/quic_spdy_stream.h
[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/reliable_quic_stream_test.cc
[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/test_tools/quic_session_peer.cc
[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/test_tools/quic_session_peer.h
[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/test_tools/quic_test_utils.cc
[modify] https://crrev.com/72c4e825fc4c65a8898ac3b3bf53092aba9787e3/net/quic/test_tools/quic_test_utils.h

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

commit 90daabce88e115895b4912adc84f79269715d227
Author: jbudorick <jbudorick@chromium.org>
Date: Thu May 12 01:36:49 2016

Revert of Fixes use-after-free bug in QUIC. (patchset #1 id:1 of https://codereview.chromium.org/1975483003/ )

Reason for revert:
breaks M51: https://bugs.chromium.org/p/chromium/issues/detail?id=611267

Original issue's description:
> Fixes use-after-free bug in QUIC.
>
> QuicSession destructor ends up calling the QuicSpdyStream destructor, which in turn tries to access a QuicSession method via a non-owning pointer back to the session. This CL arranges for the non-owning pointer to be set to null before QuicSpdyStream destructor is called.
>
> Merge Internal CL: 120973813
>
> Creates a MockQuicSession for use in ReliableQuicStreamTest, which currently incorrectly uses a MockQuicSpdySession.
>
> The test class being fixed in this CL uses a MockQuicSpdySession, which is a QuicSpdySession, with TestStreams, which are ReliableQuicStreams. Bad things can happen since QuicSpdySession seems to freely assume that its streams are QuicSpdyStreams. Bad things do happen, but they inexplicably seem to show up only in Chromium tests.
>
> Merge Internal CL: 120989117
>
> BUG= 605474 
> NOTRY=true
> NOPRESUBMIT=true
> Review-Url: https://codereview.chromium.org/1932653002
> Cr-Commit-Position: refs/heads/master@{#390575}
> (cherry picked from commit 4ee9d76953f16811186942971822cf12d48db7ae)

TBR=rch@chromium.org,jri@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 605474 

Review-Url: https://codereview.chromium.org/1970073002
Cr-Commit-Position: refs/branch-heads/2704@{#514}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/90daabce88e115895b4912adc84f79269715d227/net/quic/test_tools/quic_session_peer.cc

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

commit a704d1d6340b468962bf4a93ac591e64eefbe175
Author: John Budorick <jbudorick@chromium.org>
Date: Thu May 12 01:54:00 2016

Revert "Revert of Fixes use-after-free bug in QUIC. (patchset #1 id:1 of https://codereview.chromium.org/1975483003/ )"

This reverts commit 90daabce88e115895b4912adc84f79269715d227.

CL had already been reverted in cd9116b59af4a885eb335b794b7a7b3a15d82b36

BUG= 605474 ,611267
TBR=

Review URL: https://codereview.chromium.org/1972813002 .

Cr-Commit-Position: refs/branch-heads/2704@{#515}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/a704d1d6340b468962bf4a93ac591e64eefbe175/net/quic/test_tools/quic_session_peer.cc

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

commit af5cec5668c4dd03318ca5b31e100fb76f2959fb
Author: jri <jri@chromium.org>
Date: Thu May 12 22:40:40 2016

Fixes use-after-free bug in QUIC.

QuicSession destructor ends up calling the QuicSpdyStream destructor, which in turn tries to access a QuicSession method via a non-owning pointer back to the session. This CL arranges for the non-owning pointer to be set to null before QuicSpdyStream destructor is called.

Merge Internal CL: 120973813

Creates a MockQuicSession for use in ReliableQuicStreamTest, which currently incorrectly uses a MockQuicSpdySession.

The test class being fixed in this CL uses a MockQuicSpdySession, which is a QuicSpdySession, with TestStreams, which are ReliableQuicStreams. Bad things can happen since QuicSpdySession seems to freely assume that its streams are QuicSpdyStreams. Bad things do happen, but they inexplicably seem to show up only in Chromium tests.

Merge Internal CL: 120989117

BUG= 605474 
NOTRY=true
NOPRESUBMIT=true
Review-Url: https://codereview.chromium.org/1932653002
Cr-Commit-Position: refs/heads/master@{#390575}
(cherry picked from commit 4ee9d76953f16811186942971822cf12d48db7ae)

Review-Url: https://codereview.chromium.org/1975483003
Cr-Commit-Position: refs/branch-heads/2704@{#528}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/quic_session_test.cc
[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/quic_spdy_session.cc
[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/quic_spdy_stream.cc
[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/quic_spdy_stream.h
[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/reliable_quic_stream_test.cc
[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/test_tools/quic_session_peer.cc
[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/test_tools/quic_session_peer.h
[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/test_tools/quic_test_utils.cc
[modify] https://crrev.com/af5cec5668c4dd03318ca5b31e100fb76f2959fb/net/quic/test_tools/quic_test_utils.h

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot