crash in nsDeque::ObjectAt(int)

RESOLVED FIXED in Firefox 42, Firefox OS master

Status

()

Core
Networking: HTTP
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dmajor, Assigned: nhughes)

Tracking

(4 keywords)

42 Branch
mozilla42
Unspecified
Windows NT
crash, csectype-uaf, regression, sec-high
Points:
---

Firefox Tracking Flags

(firefox40 unaffected, firefox41 unaffected, firefox42 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.2r unaffected, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage], crash signature)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-814c7693-f9b6-486f-9af9-53c792150726.
=============================================================

Different signatures on different architectures due to inlining.

x86 report: bp-0009fa48-cb41-42df-8056-aaa122150726
x64 report: bp-814c7693-f9b6-486f-9af9-53c792150726

0 	xul.dll 	nsDeque::ObjectAt(int) 	xpcom/glue/nsDeque.cpp
1 	xul.dll 	mozilla::net::Http2BaseCompressor::SizeOfExcludingThis(unsigned __int64 (*)(void const*)) 	netwerk/protocol/http/Http2Compression.cpp
2 	xul.dll 	mozilla::net::HpackDynamicTableReporter::CollectReports(nsIMemoryReporterCallback*, nsISupports*, bool) 	netwerk/protocol/http/Http2Compression.cpp
(Reporter)

Comment 1

3 years ago
First seen in build 20150725030209 which makes the regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b0b3dcfa5557&tochange=d3228c82badd and I bet it's bug 986302.

Based on the 0x5a5a... poison value, it looks like HpackDynamicTableReporter's mCompressor has been freed. nhughes and nwgh, could you please investigate?
Blocks: 986302
Flags: needinfo?(nhughes)
Flags: needinfo?(hurley)
status-firefox41: --- → unaffected
status-firefox42: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
Keywords: csectype-uaf, regression, sec-high
Well, this is slightly disconcerting, as the memory reporter should be unregistered (and therefore unavailable to about:memory) when the compressor (or decompressor) goes away (which is when the h2 session goes away, which is when the nsHttpConnection goes away).

Regardless, when the compressor/decompressor is going away, we should null out mDynamicReporter->mCompressor, as well as nulling out mDynamicReporter (and of course checking mCompressor != nullptr before accessing it in the dynamic reporter).

nhughes, can you write that patch?
Flags: needinfo?(hurley)
(Assignee)

Comment 3

3 years ago
(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #2)
> nhughes, can you write that patch?

nwgh, sure thing, I'm on it.
Flags: needinfo?(nhughes)
When the reporting occurs the memory reporter manager grabs strong references to all the reporters and then iterates through them. So we might be getting unlucky with the timing here like so:

- Start doing memory reports;

- The compressor is destroyed... but the reporter stays alive because it's refcounted and the manager is holding a strong ref to it;

- The compressor reporter has CollectReports() called on it.

Nulling everything definitely seems like a good idea.

In general it's better for a class to be its own reporter where possible, i.e. implement nsIMemoryReporter itself instead of having a pointer to a separate reporter object, because that helps avoid these kind of lifetime issues. In this case that would have required making Http2BaseCompressor a refcounted class (i.e. a subclass of nsISupports) so I didn't suggest it. Perhaps I should have... if that's not too distasteful, it might be a better path forward.
(Assignee)

Comment 5

3 years ago
(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #2)
>
> Regardless, when the compressor/decompressor is going away, we should null
> out mDynamicReporter->mCompressor, as well as nulling out mDynamicReporter
> (and of course checking mCompressor != nullptr before accessing it in the
> dynamic reporter).
> 
> nhughes, can you write that patch?

What's the best way to null out mCompressor (private member of mDynamicReporter) from Http2BaseCompressor::~Http2BaseCompressor()? It seems sensible to just do it in ~HpackDynamicTableReporter() and call delete(mDynamicReporter) but that would require making the destructor public...
Put it in the dtor, and when you do mDynamicReporter = nullptr, the dtor will be called (assuming something else isn't holding a reference to the reporter - if it is, then we've found yet another problem).

Alternatively, you could make Http2BaseCompressor a friend of HpackDynamicTableReporter (search around in netwerk/ for syntax examples - we do that a lot) and then you can just do mDynamicReporter->mCompressor = nullptr
(In reply to Nicholas Nethercote [:njn] from comment #4)
> When the reporting occurs the memory reporter manager grabs strong
> references to all the reporters and then iterates through them. So we might
> be getting unlucky with the timing here like so:
> 
> - Start doing memory reports;
> 
> - The compressor is destroyed... but the reporter stays alive because it's
> refcounted and the manager is holding a strong ref to it;
> 
> - The compressor reporter has CollectReports() called on it.

Yeah, this is kind of what I assumed happened. Gotta love those races.

> Nulling everything definitely seems like a good idea.
> 
> In general it's better for a class to be its own reporter where possible,
> i.e. implement nsIMemoryReporter itself instead of having a pointer to a
> separate reporter object, because that helps avoid these kind of lifetime
> issues. In this case that would have required making Http2BaseCompressor a
> refcounted class (i.e. a subclass of nsISupports) so I didn't suggest it.
> Perhaps I should have... if that's not too distasteful, it might be a better
> path forward.

This is... possible, but it does start to get into the distasteful territory (the life of an HPACK compressor/decompressor is strictly tied to the life of an Http2Session, so we didn't do the compression as pointers at all - just plain ol' members of Http2Session). I'd like to avoid it if we can, and nulling everything (with appropriate null checks) seems like the best way to do that.
(Assignee)

Comment 8

3 years ago
Created attachment 8641178 [details] [diff] [review]
hpack_table_reporting.diff
Assignee: nobody → nhughes
Status: NEW → ASSIGNED
Attachment #8641178 - Flags: review?(hurley)
Comment on attachment 8641178 [details] [diff] [review]
hpack_table_reporting.diff

Review of attachment 8641178 [details] [diff] [review]:
-----------------------------------------------------------------

Please only upload a patch that has the fixes for the original patch, not the whole thing all over again plus updates :)
Attachment #8641178 - Flags: review?(hurley)
(Assignee)

Comment 10

3 years ago
Created attachment 8641206 [details] [diff] [review]
hpack_table_reporting_fix.diff
Attachment #8641178 - Attachment is obsolete: true
Attachment #8641206 - Flags: review?(hurley)
Comment on attachment 8641206 [details] [diff] [review]
hpack_table_reporting_fix.diff

Review of attachment 8641206 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/Http2Compression.cpp
@@ +62,5 @@
>    NS_IMETHODIMP
>    CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData,
>                   bool aAnonymize) override
>    {
> +    if(mCompressor == nullptr)

if (!mCompressor) {

@@ +286,5 @@
>  
>  Http2BaseCompressor::~Http2BaseCompressor()
>  {
>    UnregisterStrongMemoryReporter(mDynamicReporter);
> +  mDynamicReporter = nullptr;

So based on the comments from njn, this is the wrong way to go about this - if the suspicion is correct that we're racing the collector grabbing a strong ref to the reporter and this destructor nulling it out, then nulling mDynamicReporter here will *not* result in the reporter's dtor being called. Instead, we need to explicitly set mDynamicReporter->mCompressor = nullptr here, so just use the friend method I mentioned in my previous comment.
Attachment #8641206 - Flags: review?(hurley)
(Assignee)

Comment 12

3 years ago
Created attachment 8641247 [details] [diff] [review]
hpack_table_reporting_fix.diff
Attachment #8641206 - Attachment is obsolete: true
Attachment #8641247 - Flags: review?(hurley)
Comment on attachment 8641247 [details] [diff] [review]
hpack_table_reporting_fix.diff

Review of attachment 8641247 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a couple nits that can be fixed before setting checkin-needed.

As far as commit message goes, I think something like the following will work:

Bug 1188421 - Properly null memory reporter's compressor reference. r=hurley

::: netwerk/protocol/http/Http2Compression.cpp
@@ +54,5 @@
>  {
>  public:
>    NS_DECL_THREADSAFE_ISUPPORTS
>  
> +  friend Http2BaseCompressor;

Put this under private - I'm not sure it matters (it probably does... C++ *fistshake*), but regardless, that seems to be our normal usage.

@@ +64,5 @@
>    NS_IMETHODIMP
>    CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData,
>                   bool aAnonymize) override
>    {
> +    if(!mCompressor)

Space before the (, and put the { on the same line, separated from the ) by a space like so:

if (!mCompressor) {
Attachment #8641247 - Flags: review?(hurley) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8641275 [details] [diff] [review]
hpack_table_reporting_fix.diff
Attachment #8641247 - Attachment is obsolete: true
Attachment #8641275 - Flags: review?(hurley)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Comment on attachment 8641275 [details] [diff] [review]
hpack_table_reporting_fix.diff

Review of attachment 8641275 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/Http2Compression.cpp
@@ +62,5 @@
>    CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData,
>                   bool aAnonymize) override
>    {
> +    if (!mCompressor) {
> +      return NS_ERROR_NULL_POINTER;

This is a reasonable case, so I'd return NS_OK here.
(Assignee)

Comment 16

3 years ago
Created attachment 8641364 [details] [diff] [review]
hpack_table_reporting_fix.diff
Attachment #8641275 - Attachment is obsolete: true
Attachment #8641364 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8619079dbcb
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-master: --- → fixed
status-firefox40: --- → unaffected
status-firefox42: affected → fixed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8619079dbcb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Reporter)

Comment 19

3 years ago
Fix looks good. This crash hasn't been seen since the patch landed.
Duplicate of this bug: 1190835

Updated

3 years ago
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.