Closed Bug 1494222 Opened Last year Closed 11 months ago

minor qcms bugs

Categories

(Core :: GFX: Color Management, enhancement, minor)

enhancement
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: pdknsk, Assigned: pdknsk)

Details

Attachments

(5 files, 1 obsolete file)

751 bytes, patch
nical
: review+
Details | Diff | Splinter Review
899 bytes, patch
nical
: review+
Details | Diff | Splinter Review
826 bytes, patch
Alex_Gaynor
: review+
Details | Diff | Splinter Review
733 bytes, patch
Alex_Gaynor
: review+
Details | Diff | Splinter Review
845 bytes, patch
Alex_Gaynor
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch qcms-1Splinter Review
qcms_profile_is_bogus doesn't check bogosity of profiles with a LUT.

https://searchfox.org/mozilla-central/source/gfx/qcms/iccread.c#283

It does miss two other types of LUT-based profiles, which have the same A2B0 or B2A0 tags, but with a different LUT type.

https://searchfox.org/mozilla-central/source/gfx/qcms/iccread.c#1103
https://searchfox.org/mozilla-central/source/gfx/qcms/iccread.c#1111

So any such profile is considered bogus, unless it also contains redundant {b,g,r}XYZ tags, because otherwise the checks further down in qcms_profile_is_bogus always fail. The fuzzer can produce those profiles, but it also affects Firefox when mBA-only profiles are set as the monitor profile, which should be very rare.
Assignee: nobody → pdknsk
Status: NEW → ASSIGNED
Attached patch qcms-2Splinter Review
One of the profiles used in the target is the built-in sRGB profile, which incorrectly doesn't set mandatory field PCS, so the following code can never be reached.

https://searchfox.org/mozilla-central/source/gfx/qcms/chain.c#911
https://searchfox.org/mozilla-central/source/gfx/qcms/chain.c#931

Note this doesn't only affect the fuzzer, but also Firefox itself in rare situations: when the input profile has uncommon PCS LAB, and no monitor profile is set (which makes qcms fallback to the built-in sRGB profile).
Attached patch fuzzer-1 (obsolete) — Splinter Review
I used conditions size & 1 and size % 0x10, which excludes half the combinations.
I did a review of the oss-fuzz coverage for target Qcms and noticed some code wasn't reached that should be. That's caused by a minor bug in the target itself, and two minor bugs in qcms.

https://storage.googleapis.com/oss-fuzz-coverage/qcms/reports/20180925/linux/report.html

Those 3 patches are very simple. You you can probably review them yourself as the owner of qcms on the oss-fuzz side now. Otherwise please suggest a reviewer. Thanks.
Flags: needinfo?(agaynor)
I believe :nical has been handling qcms bugs.
Flags: needinfo?(agaynor) → needinfo?(nical.bugzilla)
Component: General → GFX: Color Management
Product: Firefox → Core
Flags: needinfo?(nical.bugzilla)
Attachment #9012086 - Flags: review?(jmuizelaar)
Attachment #9012089 - Flags: review?(jmuizelaar)
Attachment #9012090 - Flags: review?(jmuizelaar)
Comment on attachment 9012090 [details] [diff] [review]
fuzzer-1

I changed the reviewer of the patch that changes the target (as opposed to qcms) to you. It's so trivial it might not even need a review.
Attachment #9012090 - Flags: review?(jmuizelaar) → review?(agaynor)
Is there anything more to this patch than "take this branch 14/15 times instead of 15/16" so that it gets covered better?
Yes. It's because I use size & 1 elsewhere, which makes that combination static.

>>> 16 & 1
0
>>> 16 % 16
0
>>> 32 & 1
0
>>> 32 % 16
0

With an odd number, it alternates between odd and even.

>>> 15 & 1
1
>>> 15 % 15
0
>>> 30 & 1
0
>>> 30 % 15
0
Ah great, can you expand the commit message to include this detail?
Is that sufficient or do you want a more detailed explanation in the commit comment?

Bug 1494222: increase qcms_fuzzer coverage by enabling more path combinations
Or actually, this is more accurate.

Bug 1494222: fix qcms_fuzzer coverage by enabling all path combinations
Attached patch fuzzer-1+Splinter Review
Attachment #9012771 - Flags: review?(agaynor)
Attachment #9012090 - Attachment is obsolete: true
Attachment #9012090 - Flags: review?(agaynor)
Attached patch profile-1Splinter Review
Adds another profile to cover a specific path the fuzzer hasn't reached yet.

https://storage.googleapis.com/oss-fuzz-coverage/qcms/reports/20180927/linux/src/mozilla-central/gfx/qcms/chain.c.html#L784
Attachment #9012772 - Flags: review?(agaynor)
With the pending changes, qcms has full coverage. The only blocks not covered are unused utility functions, plus non-SSE and SSE1 paths, which are chosen at runtime based on CPU capabilities. I don't think Firefox runs on any CPU now that supports SSE1 but not SSE2. Even if, those paths are almost identical. Non-SSE runs on ARM, but qcms is disabled on Mobile Firefox.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#292
Attachment #9012771 - Flags: review?(agaynor) → review+
Attachment #9012772 - Flags: review?(agaynor) → review+
It appears the reviewer is busy. I can also explain those simple patches in more detail to you or someone else.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 9012086 [details] [diff] [review]
qcms-1

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

Apologies for the review lag.
Attachment #9012086 - Flags: review?(jmuizelaar) → review+
Attachment #9012089 - Flags: review?(jmuizelaar) → review+
QA Contact: dbolter
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4869ae749ced
don't check bogosity of profiles with LUT-types mAB/mBA. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1354102f8a8
set XYZ PCS on internal RGB profiles. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/25183982ae1b
fix qcms_fuzzer coverage by enabling all path combinations. r=agaynor
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e369fb83b5
add combined mAB/mBA profile to qcms_fuzzer samples. r=agaynor
Keywords: checkin-needed
I forgot a patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch qcms-3Splinter Review
Two functions have blocks which cannot be reached right now, because they depend on source pixel values. Like R>G>B and other combinations. Adds more bytes to cover those.

https://storage.googleapis.com/oss-fuzz-coverage/qcms/reports/20181007/linux/src/mozilla-central/gfx/qcms/transform.c.html#L612
Attachment #9015164 - Flags: review?(agaynor)
Attachment #9015164 - Flags: review?(agaynor) → review+
qcms-3 only. Thanks.
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd19353d27c
Add source bytes to fix qcms_fuzzer coverage. r=agaynor
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1fd19353d27c
Status: REOPENED → RESOLVED
Closed: 11 months ago11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.