Closed Bug 1355680 Opened 3 years ago Closed 3 years ago

Crash in mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter from nsHttpResponseHead::ContentLength

Categories

(Core :: Networking: HTTP, defect, critical)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 + fixed
firefox55 + fixed

People

(Reporter: ting, Assigned: schien)

References

Details

(Keywords: crash, regression, Whiteboard: [necko-active])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-fc80605a-8cf6-455d-81ac-057dc2170410.
=============================================================
Top #15 of Aurora 20170409004009 on Windows, 11 crashes from 2 installations. The same signature can also be found on Nightly.
Summary: Crash in mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter → Crash in mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter from nsHttpResponseHead::ContentLength
This should be a regression of bug 1325915.

mReentrantMonitor in nsHttpResponseHead is used after free, which means we are using a bogus pointer of nsHttpResponseHead.

Maybe I should use HttpBaseChannel::GetContentLength instead of directly access |mResponseHead| in https://hg.mozilla.org/releases/mozilla-aurora/annotate/20c110248317/netwerk/protocol/http/HttpChannelChild.cpp#l710 and assign -1 to |progressMax| if failed.

@mayhemer how do you think?
Assignee: nobody → schien
Blocks: 1325915
Flags: needinfo?(honzab.moz)
Keywords: regression
Whiteboard: [necko-active]
I think [@ mozilla::net::nsHttpResponseHead::ContentLength ] is the Linux version of this crash.
Crash Signature: [@ mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter] → [@ mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter][@ mozilla::net::nsHttpResponseHead::ContentLength ]
[@ <name omitted> | mozilla::net::HttpChannelChild::OnTransportAndData ] looks like the OSX signature.
Crash Signature: [@ mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter][@ mozilla::net::nsHttpResponseHead::ContentLength ] → [@ mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter][@ mozilla::net::nsHttpResponseHead::ContentLength ] [@ <name omitted> | mozilla::net::HttpChannelChild::OnTransportAndData ]
OS: Windows 10 → All
Thanks @mccr8! This crash is definitely affecting Firefox 54. @mayhemer are you able to provide some feedback soon or maybe @dragana can jump in to help the review?
Flags: needinfo?(dd.mozilla)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #1)
> This should be a regression of bug 1325915.
> 
> mReentrantMonitor in nsHttpResponseHead is used after free, which means we
> are using a bogus pointer of nsHttpResponseHead.
> 
> Maybe I should use HttpBaseChannel::GetContentLength instead of directly
> access |mResponseHead| in
> https://hg.mozilla.org/releases/mozilla-aurora/annotate/20c110248317/netwerk/
> protocol/http/HttpChannelChild.cpp#l710 and assign -1 to |progressMax| if
> failed.
> 
> @mayhemer how do you think?

Can you explain what is happening?
mResponseHead is set in OnStartRequest. OnStartRequest must have been called before OnTransportAndData. mResponseHead is never nulled, as I can see from a quick look?
(In reply to Dragana Damjanovic [:dragana] from comment #6)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #1)
> > This should be a regression of bug 1325915.
> > 
> > mReentrantMonitor in nsHttpResponseHead is used after free, which means we
> > are using a bogus pointer of nsHttpResponseHead.
> > 
> > Maybe I should use HttpBaseChannel::GetContentLength instead of directly
> > access |mResponseHead| in
> > https://hg.mozilla.org/releases/mozilla-aurora/annotate/20c110248317/netwerk/
> > protocol/http/HttpChannelChild.cpp#l710 and assign -1 to |progressMax| if
> > failed.
> > 
> > @mayhemer how do you think?
> 
> Can you explain what is happening?
> mResponseHead is set in OnStartRequest. OnStartRequest must have been called
> before OnTransportAndData. mResponseHead is never nulled, as I can see from
> a quick look?

mResponseHead is not always set because it depends on other flags. https://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/netwerk/protocol/http/HttpChannelChild.cpp#474

Actually nearly all the places use mResponseHead in  have a null check for it. (take HttpBaseChannel::GetContentLength as an example: https://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/netwerk/protocol/http/HttpBaseChannel.cpp#703)

In addition HttpBaseChannel::GetContentLength also shows the content length for alt data is not store in mResponseHead, so it would be natural to use that function to align all different cases.
Comment on attachment 8857348 [details]
Bug 1355680 - set progressMax to -1 when HTTP response head is not available.

https://reviewboard.mozilla.org/r/129332/#review132934

Thanks for the explanation!
Attachment #8857348 - Flags: review+
[Tracking Requested - why for this release]: crash regression
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #1)
> This should be a regression of bug 1325915.
> 
> mReentrantMonitor in nsHttpResponseHead is used after free

It more seems to me like a null deref (NOT use after free), which is consistent with the patch and its explanation.
Flags: needinfo?(honzab.moz)
Comment on attachment 8857348 [details]
Bug 1355680 - set progressMax to -1 when HTTP response head is not available.

https://reviewboard.mozilla.org/r/129332/#review132956

Thanks!

::: netwerk/protocol/http/HttpChannelChild.cpp:759
(Diff revision 1)
>  
>    // Hold queue lock throughout all three calls, else we might process a later
>    // necko msg in between them.
>    AutoEventEnqueuer ensureSerialDispatch(mEventQ);
>  
> -  const int64_t progressMax = mResponseHead->ContentLength();
> +  int64_t progressMax = 0;

do you really need to init to 0?

::: netwerk/protocol/http/HttpChannelChild.cpp:760
(Diff revision 1)
>    // Hold queue lock throughout all three calls, else we might process a later
>    // necko msg in between them.
>    AutoEventEnqueuer ensureSerialDispatch(mEventQ);
>  
> -  const int64_t progressMax = mResponseHead->ContentLength();
> +  int64_t progressMax = 0;
> +  if(NS_FAILED(GetContentLength(&progressMax))) {

nit: if (
Attachment #8857348 - Flags: review?(honzab.moz) → review+
Tracking 54/55+ for this Necko crash, which is a regression.
(In reply to Honza Bambas (:mayhemer) from comment #10)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #1)
> > This should be a regression of bug 1325915.
> > 
> > mReentrantMonitor in nsHttpResponseHead is used after free
> 
> It more seems to me like a null deref (NOT use after free), which is
> consistent with the patch and its explanation.

Yes should be null deref because the crash dump shows a very small number is used as an address.
Flags: needinfo?(dd.mozilla)
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5908d5fd0aa3
set progressMax to -1 when HTTP response head is not available. r=dragana,mayhemer
https://hg.mozilla.org/mozilla-central/rev/5908d5fd0aa3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8857348 [details]
Bug 1355680 - set progressMax to -1 when HTTP response head is not available.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1325915
[User impact if declined]: might crash
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]:  add null check
[String changes made/needed]: no
Attachment #8857348 - Flags: approval-mozilla-aurora?
Comment on attachment 8857348 [details]
Bug 1355680 - set progressMax to -1 when HTTP response head is not available.

Fix a potential crash. Aurora54+.
Attachment #8857348 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #17)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Shih-Chiang Chien's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.