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

RESOLVED FIXED in mozilla10



9 years ago
5 months ago


(Reporter: timeless, Assigned: yati.sagade)


(Blocks: 1 bug, {coverity})


Firefox Tracking Flags

(Not tracked)


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


(1 attachment)



9 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:

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]

Comment 2

7 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:

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

Comment 4

7 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+

Comment 7

7 years ago
I've added a comment at the top of the function. Please let me know if anything more needs to be added.

Comment 9

7 years ago

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 :-)
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Blocks: 1230156
You need to log in before you can comment on or make changes to this bug.