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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Ting-Yu Chou [:ting] (away)]

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.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1355680 - set progressMax to -1 when HTTP response head is not available.

Review commit: https://reviewboard.mozilla.org/r/129332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129332/

Comment 3

[Andrew McCreight [:mccr8]]

I think [@ mozilla::net::nsHttpResponseHead::ContentLength ] is the Linux version of this crash.

Comment 4

[Andrew McCreight [:mccr8]]

[@ <name omitted> | mozilla::net::HttpChannelChild::OnTransportAndData ] looks like the OSX signature.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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?

Comment 6

[Dragana Damjanovic [:dragana]]

(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?

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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

[Dragana Damjanovic [:dragana]]

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!

Comment 9

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: crash regression

Comment 10

[Honza Bambas (:mayhemer)]

(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.

Comment 11

[Honza Bambas (:mayhemer)]

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 (

Comment 12

[Marcia Knous [:marcia]]

Tracking 54/55+ for this Necko crash, which is a regression.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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.

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8857348 [details]
Bug 1355680 - set progressMax to -1 when HTTP response head is not available.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129332/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5908d5fd0aa3
set progressMax to -1 when HTTP response head is not available. r=dragana,mayhemer

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5908d5fd0aa3

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8857348 [details]
Bug 1355680 - set progressMax to -1 when HTTP response head is not available.

Fix a potential crash. Aurora54+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/631c3b43f95c

Comment 20

[Andrei Vaida [:avaida]]

(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.