Closed
Bug 1355680
Opened 6 years ago
Closed 6 years ago
Crash in mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter from nsHttpResponseHead::ContentLength
Categories
(Core :: Networking: HTTP, defect)
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)
59 bytes,
text/x-review-board-request
|
mayhemer
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
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.
Reporter | ||
Updated•6 years ago
|
status-firefox54:
--- → affected
status-firefox55:
--- → affected
Summary: Crash in mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter → Crash in mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter from nsHttpResponseHead::ContentLength
Assignee | ||
Comment 1•6 years ago
|
||
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?
Comment hidden (mozreview-request) |
Updated•6 years ago
|
status-firefox52:
--- → unaffected
status-firefox53:
--- → unaffected
Updated•6 years ago
|
Whiteboard: [necko-active]
Comment 3•6 years ago
|
||
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 ]
Comment 4•6 years ago
|
||
[@ <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
Assignee | ||
Comment 5•6 years ago
|
||
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)
Comment 6•6 years ago
|
||
(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?
Assignee | ||
Comment 7•6 years ago
|
||
(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 8•6 years ago
|
||
mozreview-review |
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+
Comment 9•6 years ago
|
||
[Tracking Requested - why for this release]: crash regression
tracking-firefox54:
--- → ?
tracking-firefox55:
--- → ?
![]() |
||
Comment 10•6 years ago
|
||
(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 11•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 13•6 years ago
|
||
(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.
Updated•6 years ago
|
Flags: needinfo?(dd.mozilla)
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
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
![]() |
||
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5908d5fd0aa3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 17•6 years ago
|
||
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 18•6 years ago
|
||
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+
Comment 19•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/631c3b43f95c
Updated•6 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 20•6 years ago
|
||
(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.
Description
•