Closed Bug 550969 Opened 13 years ago Closed 11 years ago

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


(Core :: Graphics: Color Management, defect)

Not set





(Reporter: timeless, Assigned: yati.sagade)


(Blocks 1 open bug, )


(Keywords: coverity, Whiteboard: [good first bug][])


(1 file)

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][]
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
Attached patch patch v1Splinter Review
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+
I've added a comment at the top of the function. Please let me know if anything more needs to be added.

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