The default bug view has changed. See this FAQ.

white_point_from_temp doesn't handle out of range T values in opt builds

RESOLVED FIXED in mozilla10

Status

()

Core
GFX: Color Management
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: timeless, Assigned: Yati Sagade)

Tracking

({coverity})

Trunk
mozilla10
x86
Linux
coverity
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=bgirard@mozilla.com], URL)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
596  	static qcms_CIE_xyY white_point_from_temp(int temp_K)

599  		double x, y;

609  		if (T >= 4000. && T <= 7000.) {
611  		} else {

613  			if (T > 7000.0 && T <= 25000.0) {
615  			} else {

if we reach here, then we're fine in debug builds, because the app kills itself, but in release builds, we'll go past this point leaving x uninitialized:
616  				assert(0 && "invalid temp");

and use it here:
622  		y = -3.000*(x*x) + 2.870*x - 0.275;

and return them:
630  		white_point.x = x;
631  		white_point.y = y;

634  		return white_point;
Willing to mentor this change:
The problem is gfx/qcms/iccread.c. Qcms is our color management library that converts image color space. See this article for more information:
http://www.cambridgeincolour.com/tutorials/color-space-conversion.htm

The problem in this bug is that if the parameter temp_K is in an invalid range we will return undefined values. assert are omitted in release builds so we fall through and read uninitialized memory.

We should set the white_point to D50 white-point after that assert (x = 0.3457, y = 0.3585). A sane default for an invalid parameter.
Whiteboard: [good first bug][mentor=bgirard@mozilla.com]
(Assignee)

Comment 2

6 years ago
I'd like to work on this.
Thanks Yati. I've posted some information in Comment 1.

The first step will be to get the source and compile it:
https://developer.mozilla.org/En/Simple_Firefox_build

then take a look at gfx/qcms/iccread.c.

Feel free to post any question you have here on the mean time or find me on IRC.
Assignee: nobody → yati.sagade
(Assignee)

Comment 4

6 years ago
Created attachment 566621 [details] [diff] [review]
patch v1
Comment on attachment 566621 [details] [diff] [review]
patch v1

Patch looks good to me, passing the review to Jeff since he's the module owner for Color management.
Attachment #566621 - Flags: review?(jmuizelaar)
Attachment #566621 - Flags: feedback+
Comment on attachment 566621 [details] [diff] [review]
patch v1

I originally thought returning the D50 white point made more sense but returning -1, -1, -1 will probably make the wrong input more noticeable to the caller. It's probably worth adding a comment about this rationale.
Attachment #566621 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 7

6 years ago
I've added a comment at the top of the function. Please let me know if anything more needs to be added.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa4a5905d1a
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/4aa4a5905d1a

Congratulations on your first patch in the tree! Hope to see you on IRC (http://irc.mozilla.org/) in #developers soon. If you'd like to fix another bug (we'd love it if you did!) but need some inspiration, pop on & say hi - and we'll find something for you :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.