Last Comment Bug 550969 - white_point_from_temp doesn't handle out of range T values in opt builds
: white_point_from_temp doesn't handle out of range T values in opt builds
[good first bug][mentor=bgirard@mozil...
: coverity
Product: Core
Classification: Components
Component: GFX: Color Management (show other bugs)
: Trunk
: x86 Linux
-- normal (vote)
: mozilla10
Assigned To: Yati Sagade
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2010-03-08 11:47 PST by timeless
Modified: 2011-10-15 05:18 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1 (1.37 KB, patch)
2011-10-12 13:56 PDT, Yati Sagade
jmuizelaar: review+
b56girard: feedback+
Details | Diff | Splinter Review

Description User image timeless 2010-03-08 11:47:31 PST
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;
Comment 1 User image Benoit Girard (:BenWa) 2011-10-07 11:54:06 PDT
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.
Comment 2 User image Yati Sagade 2011-10-11 14:28:27 PDT
I'd like to work on this.
Comment 3 User image Benoit Girard (:BenWa) 2011-10-11 14:59:00 PDT
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.
Comment 4 User image Yati Sagade 2011-10-12 13:56:21 PDT
Created attachment 566621 [details] [diff] [review]
patch v1
Comment 5 User image Benoit Girard (:BenWa) 2011-10-12 22:42:16 PDT
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.
Comment 6 User image Jeff Muizelaar [:jrmuizel] 2011-10-13 05:42:01 PDT
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.
Comment 7 User image Yati Sagade 2011-10-13 11:54:47 PDT
I've added a comment at the top of the function. Please let me know if anything more needs to be added.
Comment 9 User image Ed Morley [:emorley] 2011-10-15 05:18:25 PDT

Congratulations on your first patch in the tree! Hope to see you on IRC ( 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 :-)

Note You need to log in before you can comment on or make changes to this bug.