458164 - Crash by opening special images in browser or mailnews

Status: VERIFIED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Hartmut Figge]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; rv:1.9.1b1pre) Gecko/20081002 Mnenhy/0.7.5.20005.h1 SeaMonkey/2.0a2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; rv:1.9.1b1pre) Gecko/20081002 Mnenhy/0.7.5.20005.h1 SeaMonkey/2.0a2pre

The problem was discovered reading postings with attached images in the newsgroup de.alt.dateien.misc. An example is Message-ID: <gbvq7d$tha$[e-mail]>.

Opening the posting resulted in a crash, which was confirmed by other users. I have uploaded some of the offending images to make reproduction of the crash easier.


Reproducible: Always

Steps to Reproduce:
1. Use a Nighly of SM 2.0 newer than 2008-09-12 07:09:00 PDT
2. *important* Restart SM
3. Open one of the images in
http://www.triffids.de/pub/sm/2.0/bugs/photo_crash/

Actual Results:  
As a result SM crashes with this message on the terminal:
lcms: Error #12288; Read from memory error. Got 128 bytes, block should
be of 128 bytes


Expected Results:  
Expected is obviously no crash but the display of the image. ;)

Last good of my self compiled builds is 2008-09-10 15:02:00 PDT, first bad is 2008-09-12 07:09:00 PDT. I have also tested a regular nightly and the RC of SM2.

The crash could be seen by others on Windows NT 5.1, Windows NT 6.0 and Mac OS X 10.4. Additional information is in the thread 'Strange crashes' in mozilla.dev.apps.seamonkey, starting with <[e-mail]>.

Funny thing is, that the crash can be prevented by opening http://www.triffids.de/pub/sm/2.0/bugs/photo_crash/prevent_crash/Test1.jpg after step 2 and before step 3.

Because only certain images causes this crash, all from the same poster in de.alt.dateien.misc, i should append what this user has written:

| All i have done is to scale down the images with the VSO Image
| Resizer. Now i have appended one which was scaled down with the
| Microsoft Image Manager

The appended one is the above mentioned Test1.jpg.

Comment 1

[Tobias Fischer]

Attachment: Screenshot from Error-Popups when try to open image

Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20081001 Mnenhy/0.7.5.20005 SeaMonkey/2.0a2pre

I can confirm this Bug so far, that there is something wrong handling the named images in SeaMonkey using the "Modern" Theme. Here on Windows XP SeaMonkey will not really crash, but was terminated from an Error in Little cms, as the Screenshot shows. No Errors in Error-Console.

On MacOS X 10.4.x the SeaMonkey 2008100103-Nightly just crash when try to open an Image, without Breakpad coming up or any other error message. 

After some crashes (unplanned terminations from lcms) using the "Modern" Theme now I get the same Error with the SeaMonkey-Default Theme. Before I have tested this with "Modern", I got no Errors with the Default-Theme, the Images were open without problems in Browser. Hmm.

Comment 2

[Hartmut Figge]

(In reply to comment #1)

> After some crashes (unplanned terminations from lcms) using the "Modern" Theme
> now I get the same Error with the SeaMonkey-Default Theme. Before I have tested
> this with "Modern", I got no Errors with the Default-Theme, the Images were
> open without problems in Browser.

Well, same thing here on Linux. First there were crashes on Modern and not on Classic, but after some time of testing things changed. Now the crashes occur on both themes.

> Hmm.

Yes. Hmmm. *g*

Comment 3

[Barry Edwin GIlmour]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20081001 SeaMonkey/2.0a2pre ID:20081001010949 Mandriva-2008.1-PwrPack Linux-2.6.24.7-desktop586-1mnb
For the first couple of tries, my restarted bare-profile did not elicit this crash in either theme, but when I repeated the tests in both the default and modern themes again, both themes now-crash faster than thought. Something noticeable is that the "alternate-image-text" loads cleanly in the window, but as the image starts to draw-itself into the window, SM crashes instantly, as-though the JPG-file is carrying a conflicting-piece of code, or something. Is neutralisation-code in the enabling-image? The bug's effect does accumulate with exposure! whatever the code is, it is definitely a savage conflict!

Comment 4

[[e-mail]]

What's happening here isn't a true crash because the lcms decoder calls exit so the crash never shows up on the crash reports. I saw dbaron mentioning something similar on IRC but I apologise if that isn't in fact related.

There is a JPEG marker in the file whose data is ICC_PROFILE\0\1\1\0 which decodes as "ICC_PROFILE" (char)1 (char)1 i.e. first of 1 ICC profile marker. The last byte is then assumed to be the ICC profile itself. However a 1-byte ICC profile is invalid, as the minimum size is 128 bytes.

Comment 5

[Christian Eyrich]

Attachment: proposed patch

This patch adds two lines of defence. First it looks right after detecting the APP marker if the minimum amount of data for a profile is available.

Secondly the default error handling of abort is temporarely changed to show when opening the profile. This switching via cmsErrorAction() is alredy done on several places in the lcms code and thus should be ok.

Finally I think there was a bug in the error reported.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 347178 [details] [diff] [review]
proposed patch

I don't know enough about LCMS to know whether this is appropriate. Luckily, I know someone who does. :)

Comment 7

[Bobby Holley (:bholley)]

Interesting.

In general, we want to completely disable the lcms error handling system - any sort of failed color management should never bring down the app. This is currently handled here:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxPlatform.cpp#572

However, this can be problematic, since the error handler isn't set until the output profile is queried, which in theory can happen after an input profile is read and created. My guess as to why this doesn't bring down Firefox is that (at least currently) some chrome images have embedded profiles so the whole color management system fires up well before any images from the web are loaded.

I think that most of the attached patch is inappropriate, since we only want to modify LCMS when it has actual bugs or prevents us from doing anything with its interface. Instead, I think we should probably most the setting of the error handler to the init routine of gfxPlatform. I'll put together a patch for that now.

Comment 8

[Bobby Holley (:bholley)]

Attachment: patch to set the error handler at gfx init

I've added a patch to set the error handler in gfx initialization.

I'd normally kick off a tryserver build and point the people in this bug to it, but I'm not sure how to do this for mailnews. Can someone try this patch and comment on whether it fixes the issue?

Comment 9

[Christian Eyrich]

> I've added a patch to set the error handler in gfx initialization.

Thanks! I understand the policy of not touching lcms whenever possible.

> Can someone try this patch and comment on whether it fixes the issue?

Here it prevents the crash while it reliably crashes without, SM trunk on Linux.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 349722 [details] [diff] [review]
patch to set the error handler at gfx init

Ok - thank christian. Flagging Joe for review.

Comment 11

[Joe Drew (not getting mail)]

Comment on attachment 349722 [details] [diff] [review]
patch to set the error handler at gfx init

r+ as long as you add a test that'll make sure we don't regress this.

Comment 12

[Bobby Holley (:bholley)]

Not sure of the testing infrastructure for mailnews.

Christian - assuming I land this, can you put something in the test suite that reproduces this issue?

Comment 13

[Joe Drew (not getting mail)]

Bobby, I presume it should be possible to trigger this even outside of mailnews?

Comment 14

[Christian Eyrich]

I'm not familiar with the test suite and how broad or specific the test should be. But a short programm which tests if code behind
  gfxPlatform::Init();
  char *profile = malloc(1);
  cmsOpenProfileFromMem(profile, 1);
is reached might be sufficient. If yes, it's ok.

Comment 15

[Bobby Holley (:bholley)]

Joe - I'm pretty sure it can't be triggered from within firefox because the color management setup seems to get initialized before any user images are loaded. As such I can't think of any way to test this with reftests/mochitests/crashtests. If there is an equivalent for mailnews, it would probably be possible to repro the issue that way.

However, assuming we can plug it in, christian's standalone code should work fine - any idea how to plug standalone tests into the testing framework?

Comment 16

[Joe Drew (not getting mail)]

Bobby -- feel free to add tests only to mozilla-central (and not the 1.9.1 branch) if it's easier.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 349722 [details] [diff] [review]
patch to set the error handler at gfx init

Requesting 191 approval for this patch. It's probably not a huge deal, but it would be a big security bug/crasher if someone figured out how to trigger this in the browser. There's not much chance for regression, since this patch just moves the code that disables fatal errors in LCMS to the earliest possible moment in thebes. I think it'd be smart to get this landed for 3.1.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349722 [details] [diff] [review]
patch to set the error handler at gfx init

a191=beltzner as long as it goes along with the tests in 466875

Comment 19

[Bobby Holley (:bholley)]

pushed to mc in d712c2f339aa.

Not closing the bug because this still needs to go to 191. Letting it bake overnight.

Comment 20

[Bobby Holley (:bholley)]

Resolving as fixed - marked as needing 191 landing

Comment 21

[Bobby Holley (:bholley)]

pushed to releases/mozilla-1.9.1 in 4b8493a09aeb

Comment 22

[Aakash Desai [:aakashd]]

verified FIXED on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090403 Shiretoko/3.5b4pre ID:20090403030624

and on trunk build: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090331 Minefield/3.6a1pre ID:20090331030637