ofz#71606 use extended upstream solution

Change-Id: Id4c511572792da1edd7ebbe15c1fb994ac30bdd4
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/173561
Reviewed-by: Caolán McNamara <caolan.mcnamara@collabora.com>
Tested-by: Jenkins
This commit is contained in:
Caolán McNamara 2024-09-17 13:41:24 +01:00
parent 9671a3d31e
commit bfb66b4ee8

View file

@ -1,27 +1,107 @@
From e06f9a3bece6130212b244ac4e1a1d316990f3c0 Mon Sep 17 00:00:00 2001
From 521e8e8f7f3ef05135380d5b755e147826364da5 Mon Sep 17 00:00:00 2001
From: John Bowler <jbowler@acm.org>
Date: Mon, 16 Sep 2024 17:30:38 -0700
Subject: [PATCH] ACES AP0 adjusted fixes
The subtracts in PNG_XYZ_from_xy might be producing integer overflow
with some valid but extreme xy values. This re-introduces the previous
checks but with less limited bounds; sufficient I believe to accomodate
any reasonable set of endpoints.
The subtracts in PNG_XYZ_from_xy are producing integer overflow with
some valid but extreme xy values. This re-introduces the previous
checks but with less limited bounds; sufficient to accomodate the
ACEScg end points (ACES AP1) but not for the ACES AP0 end points. Those
were not working anyway because libpng reads the cHRM parameters as
unsigned values so they must always be at least 0.
This is a temporary fix since it outlaws valid PNG cHRM chunks; the only
valid approaches are not to check or to using floating point arithmetic
internally.
A better solution requires recognizing reasonable negative values (ones
which violate the current spec) and allowing them too, at least on read.
Signed-off-by: John Bowler <jbowler@acm.org>
---
png.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
png.c | 156 ++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 120 insertions(+), 36 deletions(-)
diff --git a/png.c b/png.c
index 500daea5f..5d6db2974 100644
index 500daea5f..8a1e2a451 100644
--- a/png.c
+++ b/png.c
@@ -1289,6 +1289,20 @@ png_XYZ_from_xy(png_XYZ *XYZ, const png_xy *xy)
@@ -1203,22 +1203,66 @@ png_colorspace_sync(png_const_structrp png_ptr, png_inforp info_ptr)
#endif /* GAMMA */
#ifdef PNG_COLORSPACE_SUPPORTED
-static int
-png_safe_add(png_int_32 *addend0_and_result, png_int_32 addend1,
- png_int_32 addend2) {
- /* Safely add three integers. Returns 0 on success, 1 on overlow.
+static png_int_32
+png_fp_add(png_int_32 addend0, png_int_32 addend1, int *error)
+{
+ /* Safely add two fixed point values setting an error flag and returning 0.5
+ * on overflow.
* IMPLEMENTATION NOTE: ANSI requires signed overflow not to occur, therefore
* relying on addition of two positive values producing a negative one is not
* safe.
*/
- int addend0 = *addend0_and_result;
- if (0x7fffffff - addend0 < addend1)
- return 1;
- addend0 += addend1;
- if (0x7fffffff - addend1 < addend2)
- return 1;
- *addend0_and_result = addend0 + addend2;
- return 0;
+ if (addend0 > 0)
+ {
+ if (0x7fffffff - addend0 >= addend1)
+ return addend0+addend1;
+ }
+ else if (addend0 < 0)
+ {
+ if (-0x7fffffff - addend0 <= addend1)
+ return addend0+addend1;
+ }
+ else
+ return addend1;
+
+ *error = 1;
+ return PNG_FP_1/2;
+}
+
+static png_int_32
+png_fp_sub(png_int_32 addend0, png_int_32 addend1, int *error)
+{
+ /* As above but calculate addend0-addend1. */
+ if (addend1 > 0)
+ {
+ if (-0x7fffffff + addend1 <= addend0)
+ return addend0-addend1;
+ }
+ else if (addend1 < 0)
+ {
+ if (0x7fffffff + addend1 >= addend0)
+ return addend0+addend1;
+ }
+ else
+ return addend0;
+
+ *error = 1;
+ return PNG_FP_1/2;
+}
+
+static int
+png_safe_add(png_int_32 *addend0_and_result, png_int_32 addend1,
+ png_int_32 addend2)
+{
+ /* Safely add three integers. Returns 0 on success, 1 on overflow. Does not
+ * set the result on overflow.
+ */
+ int error = 0;
+ int result = png_fp_add(*addend0_and_result,
+ png_fp_add(addend1, addend2, &error),
+ &error);
+ if (!error) *addend0_and_result = result;
+ return error;
}
/* Added at libpng-1.5.5 to support read and write of true CIEXYZ values for
@@ -1289,6 +1333,29 @@ png_XYZ_from_xy(png_XYZ *XYZ, const png_xy *xy)
png_fixed_point red_inverse, green_inverse, blue_scale;
png_fixed_point left, right, denominator;
@ -29,19 +109,138 @@ index 500daea5f..5d6db2974 100644
+ * have end points with 0 tristimulus values (these are impossible end
+ * points, but they are used to cover the possible colors). We check
+ * xy->whitey against 5, not 0, to avoid a possible integer overflow.
+ *
+ * The limits here will *not* accept ACES AP0, where bluey is -7700
+ * (-0.0770) because the PNG spec itself requires the xy values to be
+ * unsigned. whitey is also required to be 5 or more to avoid overflow.
+ *
+ * Instead the upper limits have been relaxed to accomodate ACES AP1 where
+ * redz ends up as -600 (-0.006). ProPhotoRGB was already "in range."
+ * The new limit accomodates the AP0 and AP1 ranges for z but not AP0 redy.
+ */
+ if (xy->redx < -PNG_FP_1 || xy->redx > 2*PNG_FP_1) return 1;
+ if (xy->redy < -PNG_FP_1 || xy->redy > 2*PNG_FP_1) return 1;
+ if (xy->greenx < -PNG_FP_1 || xy->greenx > 2*PNG_FP_1) return 1;
+ if (xy->greeny < -PNG_FP_1 || xy->greeny > 2*PNG_FP_1) return 1;
+ if (xy->bluex < -PNG_FP_1 || xy->bluex > 2*PNG_FP_1) return 1;
+ if (xy->bluey < -PNG_FP_1 || xy->bluey > 2*PNG_FP_1) return 1;
+ if (xy->whitex < -PNG_FP_1 || xy->whitex > 2*PNG_FP_1) return 1;
+ if (xy->whitey < -PNG_FP_1 || xy->whitey > 2*PNG_FP_1) return 1;
+ const png_fixed_point fpLimit = PNG_FP_1+(PNG_FP_1/10);
+ if (xy->redx < 0 || xy->redx > fpLimit) return 1;
+ if (xy->redy < 0 || xy->redy > fpLimit-xy->redx) return 1;
+ if (xy->greenx < 0 || xy->greenx > fpLimit) return 1;
+ if (xy->greeny < 0 || xy->greeny > fpLimit-xy->greenx) return 1;
+ if (xy->bluex < 0 || xy->bluex > fpLimit) return 1;
+ if (xy->bluey < 0 || xy->bluey > fpLimit-xy->bluex) return 1;
+ if (xy->whitex < 0 || xy->whitex > fpLimit) return 1;
+ if (xy->whitey < 5 || xy->whitey > fpLimit-xy->whitex) return 1;
+
/* The reverse calculation is more difficult because the original tristimulus
* value had 9 independent values (red,green,blue)x(X,Y,Z) however only 8
* derived values were recorded in the cHRM chunk;
@@ -1432,18 +1499,23 @@ png_XYZ_from_xy(png_XYZ *XYZ, const png_xy *xy)
* (green-x - blue-x)*(red-y - blue-y)-(green-y - blue-y)*(red-x - blue-x)
*
* Accuracy:
- * The input values have 5 decimal digits of accuracy. The values are all in
- * the range 0 < value < 1, so simple products are in the same range but may
- * need up to 10 decimal digits to preserve the original precision and avoid
- * underflow. Because we are using a 32-bit signed representation we cannot
- * match this; the best is a little over 9 decimal digits, less than 10.
+ * The input values have 5 decimal digits of accuracy.
+ *
+ * In the previous implementation the values were all in the range 0 < value
+ * < 1, so simple products are in the same range but may need up to 10
+ * decimal digits to preserve the original precision and avoid underflow.
+ * Because we are using a 32-bit signed representation we cannot match this;
+ * the best is a little over 9 decimal digits, less than 10.
+ *
+ * This range has now been extended to allow values up to 1.1, or 110,000 in
+ * fixed point.
*
* The approach used here is to preserve the maximum precision within the
* signed representation. Because the red-scale calculation above uses the
- * difference between two products of values that must be in the range -1..+1
- * it is sufficient to divide the product by 7; ceil(100,000/32767*2). The
- * factor is irrelevant in the calculation because it is applied to both
- * numerator and denominator.
+ * difference between two products of values that must be in the range
+ * -1.1..+1.1 it is sufficient to divide the product by 8;
+ * ceil(121,000/32767*2). The factor is irrelevant in the calculation
+ * because it is applied to both numerator and denominator.
*
* Note that the values of the differences of the products of the
* chromaticities in the above equations tend to be small, for example for
@@ -1465,19 +1537,25 @@ png_XYZ_from_xy(png_XYZ *XYZ, const png_xy *xy)
* Adobe Wide Gamut RGB
* 0.258728243040113 0.724682314948566 0.016589442011321
*/
- /* By the argument, above overflow should be impossible here. The return
- * value of 2 indicates an internal error to the caller.
+ int error = 0;
+
+ /* By the argument above overflow should be impossible here, however the
+ * code now simply returns a failure code. The xy subtracts in the arguments
+ * to png_muldiv are *not* checked for overflow because the checks at the
+ * start guarantee they are in the range 0..110000 and png_fixed_point is a
+ * 32-bit signed number.
*/
- if (png_muldiv(&left, xy->greenx-xy->bluex, xy->redy - xy->bluey, 7) == 0)
+ if (png_muldiv(&left, xy->greenx-xy->bluex, xy->redy - xy->bluey, 8) == 0)
return 1;
- if (png_muldiv(&right, xy->greeny-xy->bluey, xy->redx - xy->bluex, 7) == 0)
+ if (png_muldiv(&right, xy->greeny-xy->bluey, xy->redx - xy->bluex, 8) == 0)
return 1;
- denominator = left - right;
+ denominator = png_fp_sub(left, right, &error);
+ if (error) return 1;
/* Now find the red numerator. */
- if (png_muldiv(&left, xy->greenx-xy->bluex, xy->whitey-xy->bluey, 7) == 0)
+ if (png_muldiv(&left, xy->greenx-xy->bluex, xy->whitey-xy->bluey, 8) == 0)
return 1;
- if (png_muldiv(&right, xy->greeny-xy->bluey, xy->whitex-xy->bluex, 7) == 0)
+ if (png_muldiv(&right, xy->greeny-xy->bluey, xy->whitex-xy->bluex, 8) == 0)
return 1;
/* Overflow is possible here and it indicates an extreme set of PNG cHRM
@@ -1485,29 +1563,35 @@ png_XYZ_from_xy(png_XYZ *XYZ, const png_xy *xy)
* scale value because this allows us to delay the multiplication of white-y
* into the denominator, which tends to produce a small number.
*/
- if (png_muldiv(&red_inverse, xy->whitey, denominator, left-right) == 0 ||
+ if (png_muldiv(&red_inverse, xy->whitey, denominator,
+ png_fp_sub(left, right, &error)) == 0 || error ||
red_inverse <= xy->whitey /* r+g+b scales = white scale */)
return 1;
/* Similarly for green_inverse: */
- if (png_muldiv(&left, xy->redy-xy->bluey, xy->whitex-xy->bluex, 7) == 0)
+ if (png_muldiv(&left, xy->redy-xy->bluey, xy->whitex-xy->bluex, 8) == 0)
return 1;
- if (png_muldiv(&right, xy->redx-xy->bluex, xy->whitey-xy->bluey, 7) == 0)
+ if (png_muldiv(&right, xy->redx-xy->bluex, xy->whitey-xy->bluey, 8) == 0)
return 1;
- if (png_muldiv(&green_inverse, xy->whitey, denominator, left-right) == 0 ||
+ if (png_muldiv(&green_inverse, xy->whitey, denominator,
+ png_fp_sub(left, right, &error)) == 0 || error ||
green_inverse <= xy->whitey)
return 1;
/* And the blue scale, the checks above guarantee this can't overflow but it
* can still produce 0 for extreme cHRM values.
*/
- blue_scale = png_reciprocal(xy->whitey) - png_reciprocal(red_inverse) -
- png_reciprocal(green_inverse);
- if (blue_scale <= 0)
+ blue_scale = png_fp_sub(png_fp_sub(png_reciprocal(xy->whitey),
+ png_reciprocal(red_inverse), &error),
+ png_reciprocal(green_inverse), &error);
+ if (error || blue_scale <= 0)
return 1;
- /* And fill in the png_XYZ: */
+ /* And fill in the png_XYZ. Again the subtracts are safe because of the
+ * checks on the xy values at the start (the subtracts just calculate the
+ * corresponding z values.)
+ */
if (png_muldiv(&XYZ->red_X, xy->redx, PNG_FP_1, red_inverse) == 0)
return 1;
if (png_muldiv(&XYZ->red_Y, xy->redy, PNG_FP_1, red_inverse) == 0)
--
2.46.0