943803 - (CVE-2014-1482) Image decoding causing FireFox to crash with Goo Create

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Fredrik]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.57 Safari/537.36

Steps to reproduce:

This is a bug discovered in Goo Create, the issue reported on our bugtracking system is located here : http://goo.myjetbrains.com/youtrack/issue/create-221

Crash reports reported related to this:

bp-dd9e5c6b-c6f7-46d8-afca-0847d2131121	2013-11-21	13:17
bp-b4364feb-e53b-4a1d-9444-cb0e22131121	2013-11-21	13:10
bp-3510a383-ad61-4290-821c-97ff32131121	2013-11-21	13:06
bp-98816ed0-8987-4915-92cf-257222131121	2013-11-21	13:01
bp-24d61e5d-e863-4fe4-b844-21f2d2131121	2013-11-21	13:00
bp-36f83bc5-a4ea-4e82-b392-00f5f2131121	2013-11-21	12:58
bp-2a8e1482-726a-458c-b538-6a5ed2131121	2013-11-21	12:56
bp-b1d3fe71-ef60-4942-898a-03ff02131121	2013-11-21	12:55
bp-fc8435e9-34f9-4ee2-b0ab-42f562131121	2013-11-21	12:47
bp-99e4ad95-8375-41c5-aca3-642d02131121	2013-11-21	11:20
bp-cb9351ba-c090-4d26-9e8b-55c0b2131121	2013-11-21	11:16

Computer specs :

Win 7 Home Premium 64bit , SP1
Intel i5-3450
8GB RAM
NVIDIA GeForce GTX 660

1. Go to Goo Create create.gootechnologies.com, login with an account you have been provided with. The accounts can be used simultaneously , but do create new projects to avoid possible conflicts.

2. Create a new project by clicking on the "create new project" button on the bottom right of the "manage projects" dialog which is popped up upon logging in. 

3. Click on the red asset library button in the bottom right, and click on the zombie or goon to add either of them to the scene. (A model with textures is needed)

4. Using the menu bar on the left side of the screen, find and click the material button to open up the material panel. (shiny sphere icon in the middle)

5. On the right top menu, under the project header, find and click the display textures button to be able to view the textures in the current project. (checkered square icon).

6. Click on the now visible textures , may have to click between them swiftly. In some cases it instantly craches, in some cases after a few clicks. 


Actual results:

FireFox crashes (shuts down instantly), followed by a dialog for submitting a crash report. 


Expected results:

I should still be in Goo Create, with the material panel showing settings and a preview image of the selected texture.

Comment 1

[Robert Kaiser]

This seems to be all on image decoding, so CCing Seth and Jeff, who did a lot of reviews there (Joe did most of the patches in code lines I saw those stack go through).

Comment 2

[Daniel Veditz [:dveditz]]

Hiding the bug for Seth

Comment 3

[Seth Fowler [:seth] [:s2h]]

It looks to me in most of these crash reports like we're attempting to access an imgFrame that's already been freed. Quite concerning. We've seen a number of bugs lately that result from inadequate locking in RasterImage/Decoder/etc., especially on error-related code paths, and this is very possibly another of those cases. Being cautious and treating this as a security bug (thanks Daniel!) until we understand the cause.

Comment 4

[Robert Kaiser]

Adding all the signatures seen in the comment #0 crashes, though the first two are the more frequent ones.
jsimd_ycc_rgb_convert_sse2 also has bug 823071 filed as well, BTW, maybe this is the same thing?

I see we are working with Goo Create in our gaming competition - https://blog.mozilla.org/blog/2013/12/05/unleash-the-game-creator-in-you-by-entering-our-holiday-gaming-competition/ - so if that framework is likely to trigger this, we might see more of this soon.

Seth, what's our next step here? Is someone looking int this one?

Comment 5

[Robert Kaiser]

Seth, given that this is in image decoding, is your fix in bug 896268 connected to this one here?

Comment 6

[Seth Fowler [:seth] [:s2h]]

I suspect that I know the origin of this problem, because I have found a _severe_ design flaw in RasterImage that pretty much throws all of our invariants out the window. I'm fixing a special case of it in bug 896268, because there is a special-case fix there that actually makes the code cleaner anyway.

However, the general solution is that we need to use recursive locking for mDecodingMutex in RasterImage so that we do not have to unlock while sending notifications. This is the only good option. The other three options, for reference, are:

(1) Never synchronously notify, which will hurt our page load performance.
(2) Radically rewrite RasterImage to be more safe for concurrency and parallelism using fine-grained locking. In the long term, sure, this might be a better choice. It's also a huge undertaking and a potential quagmire. Even if we ultimately want to do this, I don't think this is realistic as a solution to our immediate problems.
(3) Keep crashing. =(

I'll cook up a patch shortly that switches RasterImage over to use ReentrantMonitor. I'm currently (perhaps over-optimistically) hoping that this can be accomplished without too much pain.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Robert Kaiser (:[e-mail]) from comment #5)
> Seth, given that this is in image decoding, is your fix in bug 896268
> connected to this one here?

Yeah. I didn't see this before I wrote the above comment, but absolutely, I strongly suspect we're seeing different manifestations of the same underlying issue.

Comment 8

[Robert Kaiser]

Let's confirm this in the light of what Seth said.

Once you get to that, I'd love to see a bug tracking the fix as well as risk and timeline estimates - mostly because of the ongoing gaming competition where we promote Goo Create, which is triggering the crashes tracked here.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Alright, here's a try job for a potential patch. Let's see how things go; if everything's green, I'll clean it up and post a final version for review. I'm going to test this one pretty thoroughly since this is a BIG change, even though it's not that many lines of changed code.

https://tbpl.mozilla.org/?tree=Try&rev=34c321416ff6

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: Use a reentrant monitor instead of unlocking for notifications in RasterImage.

In this patch I've replaced RasterImage::mDecodingMutex with a ReentrantMonitor. This eliminates the requirement that we unlock before sending notifications, which was necessary because notifications could cause calls back into RasterImage to methods that would acquire the lock.

As usual recursive locking is not to be used without good reason, but until such time as we significant refactoring in RasterImage, this is the safest path forward. I strongly suspect that the crash in this bug has at its root the fact that we unlock to send notifications, which breaks every invariant that we use the decoding mutex to maintain. This patch will eliminate that problem and will thus solve not only this issue, but other mysterious imagelib crashes. The same issue has been confirmed to be the root cause of bug 896268, which is much easier to reproduce.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Thanks for the review!

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/848b57d9d85b

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/848b57d9d85b

Comment 13

[Seth Fowler [:seth] [:s2h]]

Fredrik, can you verify that the problem is now fixed with the current Nightly?

Comment 14

[Fredrik]

Hello! Unfortunately, the problem seems to still be there. Other than that, there seems to be some grave CSS and WebGL bugs as well in the nightly. Probably unrelated to this issue though. 

https://crash-stats.mozilla.com/report/index/98eb2df8-104a-47ad-9c38-caecf2131219
https://crash-stats.mozilla.com/report/index/8bc5dd21-19b3-4adf-bda4-2cd282131219
https://crash-stats.mozilla.com/report/index/1a23f571-610a-436b-9809-e90742131219
https://crash-stats.mozilla.com/report/index/24fd5c0d-f6b9-42cd-9c0f-106c52131219
https://crash-stats.mozilla.com/report/index/35debd71-a0bb-46a1-8f8a-c9b782131219

Comment 15

[Fredrik]

Sorry, the CSS problem was caused on my end. But the crashing still occurs.

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to Fredrik from comment #15)
> Sorry, the CSS problem was caused on my end. But the crashing still occurs.

Thanks Fredrik. Sounds like this problem got misdiagnosed - we fixed a real problem, but not yet the problem you're experiencing! I'm going to investigate further.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't force discard images when they have a decoder open.

I reproduced the crash and determined that there is an additional problem. We are displaying the same image on the page twice, once as a WebGL texture with non-premultiplied alpha and once as a content image with premultiplied alpha. Because we only store one decoded version of the image at a time, whenever we try to draw the content version of the image it kicks out the WebGL version, and vice versa. This causes us to want to ForceDiscard() twice in a short span of time, but CanForciblyDiscard() doesn't make sure the image is fully decoded, so when we do so the second time we hit an assert in debug builds (because the decoder is still open) or a crash in release builds.

This patch just makes CanForciblyDiscard() return false if the image is not fully decoded.

Comment 18

[Seth Fowler [:seth] [:s2h]]

There is also the issue of the performance problem caused by this repeated redecoding. I'm tackling that in bug 952354.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't force discard images when they have a decoder open.

Removed a stray printf.

Comment 20

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8350421 [details] [diff] [review]
Don't force discard images when they have a decoder open.

So this actually reverts part of http://hg.mozilla.org/mozilla-central/rev/c27db897f085 which specifically wants to stop decoding currently in progress decoding images and discard them.

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8350421 [details] [diff] [review]
Don't force discard images when they have a decoder open.

So if we do call Discard from Draw then it looks like we will null out mDecodeRequest in Discard, but the thread in Discard doesn't own mDecodingMonitor. That seems like the larger correctness issue that we need to fix.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

There is a comment in bug 944094 implying possible connection to this one?  Seth, what do you think?

Comment 23

[Timothy Nikkel (:tnikkel)]

In general maybe we should put an assert before every access to shared members about owning the mutex, there is clearly a problem here, but is it the only place?

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8350421 [details] [diff] [review]
Don't force discard images when they have a decoder open.

r- because it looks like we are not solving the real issue here: unlocked access to shared members. And also for reverting part of bug 672578.

Comment 25

[Daniel Veditz [:dveditz]]

The crash reports show we're writing to unowned memory which is usually an exploitable security problem. The ones I saw, though, are "near null" which generally isn't exploitable IFF it's guaranteed the bug always happens with a null pointer (plus fixed offset) not some other bogus value.

Comment 26

[Timothy Nikkel (:tnikkel)]

Attachment: check mDecoder

Everywhere else we guard ForceDiscard calls with mDecoder checks, I think we should do the same here. There was one other call site that was probably okay but I added the checks to be sure.

Comment 27

[Timothy Nikkel (:tnikkel)]

Fredrik, if you could give this try build a look to see if it fixes the issue it would be helpful.
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-600b56b50bf0/
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5d8fde1d9564/
(it's basically the same build, just one has linux builds only, the other has every other platform)

Comment 28

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #21)
> So if we do call Discard from Draw then it looks like we will null out
> mDecodeRequest in Discard, but the thread in Discard doesn't own
> mDecodingMonitor. That seems like the larger correctness issue that we need
> to fix.

If mDecoder is null (like the NS_ABORT_IF_FALSE's at the start of RasterImage::Discard) guarantee then there probably can't be decoder threads around, so we are probably the only thread that can acquire the monitor in that situation. But that invariant is not easy to show is true. There is a ton of access to the "locked" members as RasterImage.h claims that is completely unlocked. None of them seemed like they were an immediate problem. I tried adding all the required locks (and asserting elsewhere) but I ran into a problem. We technically need to lock in RasterImage::Discard. RasterImage::Discard is called from DiscardTracker functions that own the list mutex for the discard tracker. So in order to prevent deadlock we want to enforce that we acquire the discard tracker list mutex before the RasterImage monitor, never after. (We have asserts that check this). But we also have call chains like RasterImage::ShutdownDecoder (which asserts it is in the monitor) -> Decoder::Finish -> RasterImage::DecodingComplete -> DiscardTracker::Reset which needs to acquire the discard tracker list mutex. Untangling that so that the Reset call happens outside of the monitor is tricky, and likely makes the code harder to understand. And that is just the first call chain I came across.

Comment 29

[Fredrik]

(In reply to Timothy Nikkel (:tn) from comment #27)

Hello Timothy! Glad to inform you that I could not reproduce the texture preview crash with that build of the nightly (firefox-29.0a1.en-US.win32.installer.exe used). 

Do you have any estimate of when this fix will reach the stable version of firefox?

Comment 30

[Robert Kaiser]

Given the sec-crit nature, this happening with a tool that advertise ourselves via the gaming contest, and the fact that we seem to have a patch that helps, requesting tracking all the way up to current beta (27).

Fredrik, if the patch can get into the current beta, this fix should be in stable/release in February.

Comment 31

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

Seth, can we get this reviewed so we can get it in the release?

Comment 32

[Timothy Nikkel (:tnikkel)]

His bugzilla username says he is on PTO until the end of the this week.

Comment 33

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8358775 [details] [diff] [review]
check mDecoder

It's not clear to me that this patch actually fixes a security problem. So I'm guessing the first patch that landed in this bug fixed the sec crash (but we still crashed in a different way), and this patch fixes a crash that doesn't appear to be sec. Since this is still a sec bug, out of caution, I will use the sec-approval process.


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

A smart person could realize that there is a problem when using an image as both a regular image and as a texture in webgl if they understood the code, although that is not directly clear. I'm not sure what other specifics are needed to make this crash. Although it seems like this is a safe crash to me.


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

The checkin comment basically describes the code change, no more, no less.


Which older supported branches are affected by this flaw?

This specific code has been like this for a while, I think it's the same on all currently supported branches, so this specific problem has existed for quite a while. However, bug 716140 (multi threaded image decoding) (which has target milestone 22) probably made this more likely to come up. Other code changes also could have made this more likely to happen, I haven't put any time into determining exactly what exposed this old issue.

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

See previous answer.


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

Trivial to create.


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

In the situations where this patch would change behaviour we would have hit an NS_ABORT_IF_ELSE, so it should be mostly upside with the patch after it is verified that it fixes the initial bug.

Comment 34

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

I don't think we can give sec approval to land until the patch gets an r+

Comment 35

[Timothy Nikkel (:tnikkel)]

(In reply to Curtis Koenig [:curtisk] from comment #34)
> I don't think we can give sec approval to land until the patch gets an r+

It has r+. (I'm leaving the review req on seth so that he is in on the loop when he gets back on Monday)

Comment 36

[Timothy Nikkel (:tnikkel)]

The try build from comment 27 also had the patch for bug 958758 in it. It would be useful to know if we should expedite that patch as well, or if just this patch fixes the problems you are seeing.

Fredrik, could you please test this try build which only has the patch for this bug (builds will be there in a couple hours)? http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2ee47b392812

Comment 37

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

Comment on attachment 8358775 [details] [diff] [review]
check mDecoder

sec-approval+ for trunk.

If this isn't a sec-critical at all, we should re-rate this. If you aren't even sure if this is still a security issue, it is hard to see it as critical.

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to Al Billings [:abillings] from comment #37)
> If this isn't a sec-critical at all, we should re-rate this. If you aren't
> even sure if this is still a security issue, it is hard to see it as
> critical.

It probably was sec-critical, but the first patch that landed from his bug fixed that aspect of it (however a safe crash remained).

Comment 39

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7ba959a5d1

Comment 40

[Robert Kaiser]

FYI, we are going to build with 27.0 on Jan 27, so it would be awesome if we could get this into beta before that - earlier would be preferable so we can test in a beta before.

Comment 41

[Timothy Nikkel (:tnikkel)]

That sounds reasonable. Give this a couple days on nightly to be sure and then we can uplift it.

Comment 42

[Fredrik]

That new try-build works fine! I couldn't see any different behaviour compared to that other test build.

Comment 43

[Carsten Book [:Tomcat]]

landed on central as https://hg.mozilla.org/mozilla-central/rev/5a7ba959a5d1

Comment 44

[Timothy Nikkel (:tnikkel)]

Thanks for testing.

Comment 45

[Timothy Nikkel (:tnikkel)]

I think we can call this fixed then.

Comment 46

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #41)
> That sounds reasonable. Give this a couple days on nightly to be sure and
> then we can uplift it.

When I wrote this I forgot about the other patch in this bug that landed, the reentrant monitor patch. We'll need to uplift that one too.

Comment 47

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

Let's get 'er in!

Comment 48

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8358775 [details] [diff] [review]
check mDecoder

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

Looks good to me.

Comment 49

[Timothy Nikkel (:tnikkel)]

Seth, how do you feel about uplifting the reentrant monitor patch to beta? Risky?

Comment 50

[Lukas Blakk [:lsblakk] use ?needinfo]

Based on comment 37, am tracking this all the way to Beta but if this is too risky and also not actually sec-critical we might have to wait for the next train on this.  Please address these issues in an uplift nomination.

Comment 51

[Timothy Nikkel (:tnikkel)]

(In reply to Lukas Blakk [:lsblakk] from comment #50)
> Based on comment 37, am tracking this all the way to Beta but if this is too
> risky and also not actually sec-critical we might have to wait for the next
> train on this.  Please address these issues in an uplift nomination.

It's confusing, but there are two patches. The second fixes an issue which is likely not sec-critical. The first patch looks like it does fix a sec-critical issue.

Comment 52

[Seth Fowler [:seth] [:s2h]]

IMO the reentrant monitor patch is not all that risky to uplift. On Windows our mutexes are recursive by default anyway, so we've had essentially the same situation on our most important platform for quite some time now.

Comment 53

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8346976 [details] [diff] [review]
Use a reentrant monitor instead of unlocking for notifications in RasterImage.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): The root cause is bug 716140.
User impact if declined: Crashes, memory corruption. Bad news.
Testing completed (on m-c, etc.): On m-c for 1 month.
Risk to taking this patch (and alternatives if risky): Fairly low risk. On Windows we were already using recursive mutexes, so the real impact is restricted to moving an additional chunk of code inside the lock. If this introduced a deadlock I suspect we would have encountered it in the past month, given how hot this code path is.
String or IDL/UUID changes made by this patch: None.

Comment 54

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #51)
> It's confusing, but there are two patches. The second fixes an issue which
> is likely not sec-critical. The first patch looks like it does fix a
> sec-critical issue.

Agreed that the second is probably not sec-critical but it *does* solve a crash, and a crash in a product that we promote and partner with. IMO it's worth pushing to beta if it sticks around on m-c for a while with no issues. I'm inclined to request uplift on Monday.

Comment 55

[Robert Kaiser]

(In reply to Seth Fowler [:seth] - PTO until 1/17/2014 from comment #54)
> (In reply to Timothy Nikkel (:tn) from comment #51)
> > It's confusing, but there are two patches. The second fixes an issue which
> > is likely not sec-critical. The first patch looks like it does fix a
> > sec-critical issue.
> 
> Agreed that the second is probably not sec-critical but it *does* solve a
> crash, and a crash in a product that we promote and partner with. IMO it's
> worth pushing to beta if it sticks around on m-c for a while with no issues.
> I'm inclined to request uplift on Monday.

And I agree with Seth - esp. as this crash has erratic signatures and therefore is hard to track, we'd like to have it resolved as soon as it's reasonable, ideally in 27.

Comment 56

[bhavana bajaj [:bajaj]]

Comment on attachment 8346976 [details] [diff] [review]
Use a reentrant monitor instead of unlocking for notifications in RasterImage.

Approving given the low-risk and as it resolves a crasher.

:seth I would like this to get in tomorrow's beta 8 but don't think the sheriff will be around to land it due to US holiday.

NI'ing you to help with the branch landings.Thanks !

Comment 57

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8358775 [details] [diff] [review]
check mDecoder

If we are landing the first patch for the last better I think the ideal thing to do would be to land this patch as well (if we are planning on landing it anyway, may as well do it sooner rather than later). A few days on m-c is enough testing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): revealed by the first patch in this bug
User impact if declined: crash sometimes when webgl uses images as textures
Testing completed (on m-c, etc.): been on m-c
Risk to taking this patch (and alternatives if risky): we add two conditions to an if, if those conditions don't pass we will call NS_ABORT_IF_FALSE in ForceDiscard on the next line, so this is almost a no brainer, shouldn't make anything worse.
String or IDL/UUID changes made by this patch: none

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/15d30496c66d
https://hg.mozilla.org/releases/mozilla-aurora/rev/04e166b45ee3

https://hg.mozilla.org/releases/mozilla-beta/rev/e0ced44cd88d
https://hg.mozilla.org/releases/mozilla-beta/rev/ceb821fb9115

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a8854205bbb6
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c9f305c1d9a7

Based on comment 33, this affects esr24 as well, so we probably need to backport there as well. Timothy, what about b2g18 (v1.1)? From what you're saying, it might be possibly affected as well (albeit not as easily)? We are still backporting security fixes to that branch, so please let us know :)

Comment 59

[Timothy Nikkel (:tnikkel)]

Thanks for landing those!

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #58)
> Based on comment 33, this affects esr24 as well, so we probably need to
> backport there as well. Timothy, what about b2g18 (v1.1)? From what you're
> saying, it might be possibly affected as well (albeit not as easily)? We are
> still backporting security fixes to that branch, so please let us know :)

I think the answer to all your questions is yes, and everything else you've said is correct. At least for my patch, I'll needinfo Seth to make sure it is accurate for his patch.

Comment 60

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #59)
> Thanks for landing those!
> 
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #58)
> > Based on comment 33, this affects esr24 as well, so we probably need to
> > backport there as well. Timothy, what about b2g18 (v1.1)? From what you're
> > saying, it might be possibly affected as well (albeit not as easily)? We are
> > still backporting security fixes to that branch, so please let us know :)
> 
> I think the answer to all your questions is yes, and everything else you've
> said is correct. At least for my patch, I'll needinfo Seth to make sure it
> is accurate for his patch.

Actually before multi-threaded image decoding (bug 716140) we obviously didn't even have locking at all, so where ever that landed is where we need this.

Comment 61

[Robert Kaiser]

(In reply to Timothy Nikkel (:tn) from comment #60)
> Actually before multi-threaded image decoding (bug 716140) we obviously
> didn't even have locking at all, so where ever that landed is where we need
> this.

This landed in 22 - so I guess esr24 is affected but b2g18 not.

Comment 62

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Frederik - we've landed patches for beta FF27 and Aurora FF28. It would be great if you could try today's builds and verify that this no longer crashes. Thank you.

Comment 63

[Seth Fowler [:seth] [:s2h]]

Looks like this is landed (thanks Ryan!) so clearing the needinfo.

Comment 64

[Fredrik]

Tested beta FF27 and aurora FF28 , downloaded and installed just now. No hard crashes!

Comment 65

[Ryan VanderMeulen [:RyanVM]]

(In reply to Seth Fowler [:seth] from comment #63)
> Looks like this is landed (thanks Ryan!) so clearing the needinfo.

We still need a formal approval request for esr24 and the question answered about whether this affects b2g18 in any meaningful way or not.

Comment 66

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

Is there an ESR24 patch for this sec-critical bug? Can we get it attached and nominated if so?

Comment 67

[Timothy Nikkel (:tnikkel)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #65)
> We still need a formal approval request for esr24 and the question answered
> about whether this affects b2g18 in any meaningful way or not.

The first patch fixing a potential sec-critical does not affect b2g18. The second patch, fixing a safe crash, might affect b2g18. But I think the situation may come up a lot less frequently (or not at all?) before multi-threaded image decoding (bug 716140) (and possibly other more recent landings have made it more likely to occur as well). I'm not sure what the criteria are for b2g18, so could someone please tell me if the second patch meets them.

Similarly for esr24, we want the first patch for the sec critical, but do we want the second patch which fixes the safe crash that we will get after that is fixed?

Comment 68

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

As long as the crash is safe, I'd be willing to take the patch that addresses the sec-critical for ESR24. 

How safe is that patch?

Comment 69

[Timothy Nikkel (:tnikkel)]

(In reply to Al Billings [:abillings] from comment #68)
> As long as the crash is safe, I'd be willing to take the patch that
> addresses the sec-critical for ESR24. 
> 
> How safe is that patch?

Which patch? First (fix for sec crit) or second (fix for safe crash)?

Comment 70

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

(In reply to Timothy Nikkel (:tn) from comment #69)
 
> Which patch? First (fix for sec crit) or second (fix for safe crash)?


Fix for sec-crit

Comment 71

[Timothy Nikkel (:tnikkel)]

I'll let Seth comment on the risk of his patch then.

Comment 72

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8346976 [details] [diff] [review]
Use a reentrant monitor instead of unlocking for notifications in RasterImage.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: Crashes and/or memory corruption.
Fix Landed on Version: 27
Risk to taking this patch (and alternatives if risky): Fairly low risk. On Windows we were already using recursive mutexes, so the real impact is restricted to moving an additional chunk of code inside the lock. If this introduced a deadlock I suspect we would have encountered it in the past month, given how hot this code path is.
String or UUID changes made by this patch: None.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/301891ab2016

Comment 74

[Bogdan Maris, Desktop Test Engineering]

Is there a way to get an account faster? I applied for one in the morning but 5 hours later I did not received my privileges yet, also I wrote them at http://about.gootechnologies.com/contact asking for a faster registration. I cannot verify this fix until I get that account.

Comment 75

[Fredrik]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #74)
> Is there a way to get an account faster? I applied for one in the morning
> but 5 hours later I did not received my privileges yet, also I wrote them at
> http://about.gootechnologies.com/contact asking for a faster registration. I
> cannot verify this fix until I get that account.

You've got maili Bogdan!

Comment 76

[Bogdan Maris, Desktop Test Engineering]

I was able to crash Firefox 24.2.0 ESR pre (buildID: 20140126000501) downloaded from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-esr24/ is this going to be fixed when the next ESR will be officially released?

Comment 77

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

Yes, this has been fixed in ESR24's branch and is included in the release next week. Build should be available soon at http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/24.3.0esr-candidates/.

I would expect it to fixed in the 01/26 nightly build though. Ryan?

Comment 78

[Ryan VanderMeulen [:RyanVM]]

The crash is expected, no? In comment 68, you only approved the security fix for esr24, not the crash fix.

Comment 79

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

Duh, you're right. My bad.

Comment 80

[Timothy Nikkel (:tnikkel)]

We should look at the stack of the crash Bogdan saw in order to verify that it is indeed a safe crash.

Comment 81

[Bogdan Maris, Desktop Test Engineering]

Here are the crashes registered in 24.2.0esr pre and 24.3.0esr:

24.2.0esr pre
- bp-8b25ef40-951c-4743-a30e-5452e2140128
- bp-e89da67c-2436-4874-b6ba-dacfd2140128
- bp-7a5bfee7-5b17-494f-b229-20d5d2140128

24.3.0esr
- bp-48da6947-9466-4d53-9442-0a7922140129
- bp-df6c2da1-8dee-49fd-b7ac-45f9f2140129
- bp-9a672f57-471b-4fc4-b8c1-ddf922140129

If you need any more information please let me know.

Comment 82

[Timothy Nikkel (:tnikkel)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #81)
> - bp-df6c2da1-8dee-49fd-b7ac-45f9f2140129
> - bp-9a672f57-471b-4fc4-b8c1-ddf922140129

The first four are access violation read at 0x24, so probably safe I'm guessing.

These last two are access violation write at 0x1822d000 and 0x1cb79000. Sounds unsafe. They happen on the decoding thread. The safe crash that the "check mDecoder" patch prevents is on the main thread. So if a decoder thread is able to make progress before the main thread crashes it looks like we can unsafely crash here.

So I think we need to re-evaluate and do further uplift of the "check mDecoder" patch.

Comment 83

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

Yes, we should uplift this but it isn't making the Tuesday releases.

Comment 84

[Daniel Veditz [:dveditz]]

Comment on attachment 8358775 [details] [diff] [review]
check mDecoder

[Approval Request Comment]
User impact if declined: More crashes, possibly severe security ones
Fix Landed on Version: 27 and on
Risk to taking this patch (and alternatives if risky): This is a low-risk patch
String or UUID changes made by this patch: none

On mainline (Firefox 27 and up) we've taken a 2-part fix, but on ESR-24 we only took the larger part. We're still seeing scary crashes with this bug's STR on ESR-24 and it's /possible/ -- not guaranteed -- that the missing part will help. This patch is small, safe, and tested on mozilla-central through beta. We know it will make ESR-24 less crashy and given the smallness of the patch we probably shouldn't have skipped it in the first place.

Comment 85

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8358775 [details] [diff] [review]
check mDecoder

let's get this landed, and get builds - will talk with QA in the morning about timing this for release.

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/557a5299bc6a

Comment 87

[Bogdan Maris, Desktop Test Engineering]

I was unable to crash the new 24.3.0 ESR (build 2).