1170316 - Improve input validation for ICC profiles

Status: NEW

(Core :: Graphics, defect)

Description

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

In bug 1163740 we investigated a top crash that turned out to result from NaN values in matrices in ICC profiles.

We fixed this by rejecting the bad matrices in qcms_transform_create. However, we really should have caught this issue earlier in the pipeline.

In this bug, we'll improve input validation of ICC profiles to ensure that NaN's don't sneak into any data we use for computation.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Where were the NaN's coming from and why did they cause crashes?

Comment 2

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

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Where were the NaN's coming from and why did they cause crashes?

I don't know precisely where in the code the NaN values are coming from. It seems that corrupt ICC profiles (pretty sure it's the input profiles bundled with images that are to blame) can be parsed such that we end up with NaN values.

Bug 1163740 has all the information we know so far about the NaN-related crashes. There's much that remains unknown about the details of the crash, but we do know that bailing on color space conversion when we discover the NaN values fixes the crashes.

Comment 3

[(Away)]

In short: we do some number-crunching on those values and eventually use the result to index an array. Under normal circumstances, the math works out such that the indices are always within the array bounds, but when a NaN goes through that calculation, we try to read array index 0x80000000.

Comment 4

[(Away)]

One thing that could be done here is, at the point that these NaNs are detected in qcms_transform_create, we could VirtualAlloc a page to hold diagnostic data, copy the in_matrix and out_matrix there, then call CrashReporter::RegisterAppMemory on that page, and then crash. We could then examine that memory in the minidump.