-
Notifications
You must be signed in to change notification settings - Fork 616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid div by zero with test for bad chromaticities in RGBtoXYZ #1153
Avoid div by zero with test for bad chromaticities in RGBtoXYZ #1153
Conversation
Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
@@ -66,6 +68,11 @@ RGBtoXYZ (const Chromaticities &chroma, float Y) | |||
// X and Z values of RGB value (1, 1, 1), or "white" | |||
// | |||
|
|||
if (chroma.white.y==0.0f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better test here is to check for overflow, since very small y can cause problems, too:
if (chroma.white.x * Y >= chroma.white.y * FLT_MAX)
throw
In other words, validate that the result of the division (i.e. X) is < FLT_MAX before doing the division.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that test only work for positive numbers?
@@ -77,6 +84,16 @@ RGBtoXYZ (const Chromaticities &chroma, float Y) | |||
chroma.blue.x * (chroma.green.y - chroma.red.y) + | |||
chroma.green.x * (chroma.red.y - chroma.blue.y); | |||
|
|||
|
|||
if (d==0.0f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do the same below, although better to compute the numerators first.
{ | ||
throw std::invalid_argument("Bad chromaticities: white.y cannot be zero"); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
if (abs(chroma.white.y) <= 1 && abs(chroma.white.x*Y) >= abs(chroma.white.y) * FLT_MAX) | |
throw std::invalid_argument("Bad chromaticities: white.y cannot be zero") |
Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…mySoftwareFoundation#1153) * stop div by zero by catching bad chromaticities Signed-off-by: Peter Hillman <peterh@wetafx.co.nz> * add include to ImfChromaticities for new exception Signed-off-by: Peter Hillman <peterh@wetafx.co.nz> * better detection of division by zero Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
* stop div by zero by catching bad chromaticities Signed-off-by: Peter Hillman <peterh@wetafx.co.nz> * add include to ImfChromaticities for new exception Signed-off-by: Peter Hillman <peterh@wetafx.co.nz> * better detection of division by zero Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Address https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39084
Test case has identical x,y coordinates for all chromaticities, but RGBtoXYZ will cause a division by zero with other degenerate chromaticities. This change throws an exception before doing a divide by zero, which is undefined behavior.
Maybe there should be a method that does a more complete check that the chromaticities are valid, and Header::sanityCheck should use that to disallow writing files with nonsensical chromaticities. (Primaries must form a triangle with non-zero area, the white point must be inside that triangle, and cannot have a zero y coordinate)
RGBtoXYZ is used to read Yuv-encoded files with the Rgba interface, and to read files with non-ACES primaries using the AcesInputFile API.
Signed-off-by: Peter Hillman peterh@wetafx.co.nz