UBSan abort in ext/gd/libgd/gd_interpolation.c:937

[YuanchengJiang]

Description

The following code:

<?php
$im = imagecreatefromstring(file_get_contents(__DIR__ . '/imagecreatefromstring.gif'));
imagescale($im, 32, 32, IMG_BICUBIC);

Resulted in this output:

/php-src/ext/gd/libgd/gd_interpolation.c:937:18: runtime error: -33.9318 is outside the range of representable values of type 'unsigned char'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /php-src/ext/gd/libgd/gd_interpolation.c:937:18

PHP Version

PHP 8.4.0-dev

Operating System

ubuntu 22.04

[devnexen]

seems to have been fixed upstream, can reproduce the issue only with the bundled version.

[cmb69]

The problem is that contrib->ContribRow[x].Weights[left_channel] may be negative (the color values are always positive); negative values are deliberate. So using and casting to signed char would be appropriate; to avoid negative values when "calling" gdTrueColorAlpha() we can cast there to unsigned char. The final result should practically be the same as before.

 ext/gd/libgd/gd_interpolation.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ext/gd/libgd/gd_interpolation.c b/ext/gd/libgd/gd_interpolation.c
index b5f40b4da2..942e563bd2 100644
--- a/ext/gd/libgd/gd_interpolation.c
+++ b/ext/gd/libgd/gd_interpolation.c
@@ -926,7 +926,7 @@ static inline void _gdScaleRow(gdImagePtr pSrc,  unsigned int src_width, gdImage
 	unsigned int x;
 
     for (x = 0; x < dst_width; x++) {
-		register unsigned char r = 0, g = 0, b = 0, a = 0;
+		register signed char r = 0, g = 0, b = 0, a = 0;
         const int left = contrib->ContribRow[x].Left;
         const int right = contrib->ContribRow[x].Right;
 		int i;
@@ -934,12 +934,12 @@ static inline void _gdScaleRow(gdImagePtr pSrc,  unsigned int src_width, gdImage
 		/* Accumulate each channel */
         for (i = left; i <= right; i++) {
 			const int left_channel = i - left;
-            r += (unsigned char)(contrib->ContribRow[x].Weights[left_channel] * (double)(gdTrueColorGetRed(p_src_row[i])));
-            g += (unsigned char)(contrib->ContribRow[x].Weights[left_channel] * (double)(gdTrueColorGetGreen(p_src_row[i])));
-            b += (unsigned char)(contrib->ContribRow[x].Weights[left_channel] * (double)(gdTrueColorGetBlue(p_src_row[i])));
-			a += (unsigned char)(contrib->ContribRow[x].Weights[left_channel] * (double)(gdTrueColorGetAlpha(p_src_row[i])));
+            r += (signed char)(contrib->ContribRow[x].Weights[left_channel] * (double)(gdTrueColorGetRed(p_src_row[i])));
+            g += (signed char)(contrib->ContribRow[x].Weights[left_channel] * (double)(gdTrueColorGetGreen(p_src_row[i])));
+            b += (signed char)(contrib->ContribRow[x].Weights[left_channel] * (double)(gdTrueColorGetBlue(p_src_row[i])));
+			a += (signed char)(contrib->ContribRow[x].Weights[left_channel] * (double)(gdTrueColorGetAlpha(p_src_row[i])));
         }
-        p_dst_row[x] = gdTrueColorAlpha(r, g, b, a);
+        p_dst_row[x] = gdTrueColorAlpha((unsigned char) r, (unsigned char) g, (unsigned char) b, (unsigned char) a);
     }
 }

seems to have been fixed upstream

gd_interpolation.c differs quite a lot between upstream and our bundled ligbd. The difference in this case comes due to libgd/libgd@948bb0a (the're calculating with doubles, and clamp to unsigned char at the end; likely slower, but may indeed be more precise, and avoids the signed to unsigned casts). I assume it's worth to backport that, and see how our test suite behaves; if there are notable differences, we may consider to apply the backport only to master and maybe PHP-8.4, but to apply something like the above to stable branches).

[devnexen]

Yes I planned to try to backport first, the bundled version is so dated.