1376722 - Hit MOZ_CRASH(imgCacheValidator not thread-safe) at nsISupportsImpl.cpp:43

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

Found via bughunter and reproduced on latest trunk m-c tinderbox build on windows 7.

Affects according to bughunter linux/windows on trunk debug.

Steps to reproduce:
-> Load http://www.ceskatelevize.cz/sport/zive-vysilani/
--> Moz_Crash is hit on load 

Hit MOZ_CRASH(imgCacheValidator not thread-safe) at nsISupportsImpl.cpp:43

filing as s-s just in case

Comment 1

[Carsten Book [:Tomcat]]

Bill, Nathan: Could you take look ? thanks !

Comment 2

[Nathan Froyd [:froydnj]]

This is an imglib problem.  Maybe a recent one?  I would have suspected it would have shown up much sooner.

Comment 3

[Andrew McCreight [:mccr8]]

This looks like it could be a regression from bug 1365519, which I think added an addref of the listener off the main thread to nsUnknownDecoder::OnDataAvailable().

Comment 4

[Dragana Damjanovic [:dragana]]

I made nsUnknownDecoder be able to deliver OnDataAvailable of the main thread. But any listener in the chain can veto on that if it is not thread safe. A listener that is thread safe to receive OnDataAvailable of the main thread implements nsIThreadRetargetableStreamListener. If it is not safe to receive OnDataAvailable of the main thread it does not implement that interface or it returns an error from function CheckListenerChain.

Comment 5

[Timothy Nikkel (:tnikkel)]

I don't know this area super well but imgCacheValidator was made to subclass nsIThreadRetargetableStreamListener in https://hg.mozilla.org/mozilla-central/rev/799dffa9306b "Support OnDataAvailable and OnStopRequest off main thread for image loading". So presumably it is safe to call OnDataAvailable off the main thread on imgCacheValidator and the only problem here is that imgCacheValidator should have been made to be using threadsafe refcounting.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: threadsaferefcntimgcachevalidator

I can't reproduce the bug, so I can't be sure, but I think this should fix it. I need to go over the imgCacheValidator code to make sure that it isn't doing anything else that's not threadsafe, but it should be in good condition given that it's been used off main thread for years now.

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

I went over imgCacheValidator, the only method which can be called off main thread is OnDataAvailable, and it doesn't access any member variables, it just calls OnDataAvailable of the imgRequest,  which should be fully thread safe. So this should be the correct fix.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

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

This would be an existing problem, but imgCacheValidator::~imgCacheValidator calls imgRequest::SetValidator. If imgCacheValidator somehow gets freed off the main thread, isn't it possible to conflict with the main thread calling imgRequest::GetValidator? For example:

http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/image/imgRequestProxy.cpp#666

In imgRequestProxy::PerformClone, we first query imgRequest::GetValidator to see if it has an imgCacheValidator and then call imgCacheValidator::AddProxy if it does. Could it get freed during this window?

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 56 since this is rated sec-high. Sounds like this may affect all current versions? If so it should have sec approval before landing

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #8)
> This would be an existing problem, but imgCacheValidator::~imgCacheValidator
> calls imgRequest::SetValidator. If imgCacheValidator somehow gets freed off
> the main thread, isn't it possible to conflict with the main thread calling
> imgRequest::GetValidator? For example:

Good point. Do we have any guarantee that the networking code will release the last reference to the listener passed to SetNotificationsCallbacks and AsyncOpen2 on the main thread? Or do they have to deal with their destructor potentially running off the main thread? Since OnDataAvailable is the only method called off main thread and there needs to be a OnStopRequest call after that I think that should be a natural way to write the code.

Comment 11

[Dragana Damjanovic [:dragana]]

(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> (In reply to Andrew Osmond [:aosmond] from comment #8)
> > This would be an existing problem, but imgCacheValidator::~imgCacheValidator
> > calls imgRequest::SetValidator. If imgCacheValidator somehow gets freed off
> > the main thread, isn't it possible to conflict with the main thread calling
> > imgRequest::GetValidator? For example:
> 
> Good point. Do we have any guarantee that the networking code will release
> the last reference to the listener passed to SetNotificationsCallbacks and
> AsyncOpen2 on the main thread? Or do they have to deal with their destructor
> potentially running off the main thread? Since OnDataAvailable is the only
> method called off main thread and there needs to be a OnStopRequest call
> after that I think that should be a natural way to write the code.

OnStartRequest and OnStopRequest is called on the main thread. OnStopRequest is called after all of OnDataAvailable, this is guaranteed.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

Even if it can be freed off the main thread, I think this is all we can do inside imagelib (there would need to be a corresponding change in the owners of imgCacheValidator to ensure what I was concerned about doesn't happen).

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

one would have to find the place that is doing the off main thread addref of this class, then get the timing of that to work out to conflict with another addref on the main thread. thats pretty unlikely given the structure of this code i would guess

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

no

Which older supported branches are affected by this flaw?

caused by bug 1365519 -> 56 only

If not all supported branches, which bug introduced the flaw?

caused by bug 1365519

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

don't need

How likely is this patch to cause regressions; how much testing does it need?

safe

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

sec-approval+

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16fb069369c9