Fix potential integer overflow in rowBytes multiplications

[rootvector2]

Cast the first operand to (size_t) before multiplying two uint32_t values involving rowBytes, alphaRowBytes, or yuvRowBytes to prevent unsigned integer wrap-around on large images.

[y-guyon]

Thanks!

[rootvector2]

I've updated the arithmetic to ensure all intermediate multiplications are performed in size_t and added explicit overflow checks before allocation in codec_svt.c.

In reformat.c, both x and y are now promoted to size_t before multiplication to avoid any intermediate 32-bit overflow.

This keeps the calculations fully in the size_t domain and prevents potential wraparound prior to allocation or pointer offset computation.

[wantehchang]

Yannis: Please wait for my review.

Dexter.k: There are compiler warnings in src/codec_svt.c. I will try to suggest fixes.

[rootvector2]

@wantehchang

[wantehchang]

Dexter.k: I reviewed everything except src/reformat.c, which I will review tomorrow.

Please apply the following patch, and then read my other comments which explain the changes in my patch.

diff --git a/src/codec_aom.c b/src/codec_aom.c
index 766c69fa..81eb4b09 100644
--- a/src/codec_aom.c
+++ b/src/codec_aom.c
@@ -1213,7 +1213,7 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
             const uint32_t bytesPerRow = ((image->depth > 8) ? 2 : 1) * image->width;
             for (uint32_t j = 0; j < image->height; ++j) {
                 const uint8_t * srcAlphaRow = &image->alphaPlane[(size_t)j * image->alphaRowBytes];
-                uint8_t * dstAlphaRow = &aomImage.planes[0][j * aomImage.stride[0]];
+                uint8_t * dstAlphaRow = &aomImage.planes[0][(size_t)j * aomImage.stride[0]];
                 memcpy(dstAlphaRow, srcAlphaRow, bytesPerRow);
             }
         } else {
@@ -1237,7 +1237,7 @@ static avifResult aomCodecEncodeImage(avifCodec * codec,
 
                 for (uint32_t j = 0; j < planeHeight; ++j) {
                     const uint8_t * srcRow = &image->yuvPlanes[yuvPlane][(size_t)j * image->yuvRowBytes[yuvPlane]];
-                    uint8_t * dstRow = &aomImage.planes[yuvPlane][j * aomImage.stride[yuvPlane]];
+                    uint8_t * dstRow = &aomImage.planes[yuvPlane][(size_t)j * aomImage.stride[yuvPlane]];
                     memcpy(dstRow, srcRow, bytesPerRow);
                 }
             }
diff --git a/src/codec_avm.c b/src/codec_avm.c
index 9e09ed0c..c016eeb1 100644
--- a/src/codec_avm.c
+++ b/src/codec_avm.c
@@ -904,7 +904,7 @@ static avifResult avmCodecEncodeImage(avifCodec * codec,
             const uint32_t bytesPerRow = ((image->depth > 8) ? 2 : 1) * image->width;
             for (uint32_t j = 0; j < image->height; ++j) {
                 const uint8_t * srcAlphaRow = &image->alphaPlane[(size_t)j * image->alphaRowBytes];
-                uint8_t * dstAlphaRow = &avmImage.planes[0][j * avmImage.stride[0]];
+                uint8_t * dstAlphaRow = &avmImage.planes[0][(size_t)j * avmImage.stride[0]];
                 memcpy(dstAlphaRow, srcAlphaRow, bytesPerRow);
             }
         } else {
@@ -928,7 +928,7 @@ static avifResult avmCodecEncodeImage(avifCodec * codec,
 
                 for (uint32_t j = 0; j < planeHeight; ++j) {
                     const uint8_t * srcRow = &image->yuvPlanes[yuvPlane][(size_t)j * image->yuvRowBytes[yuvPlane]];
-                    uint8_t * dstRow = &avmImage.planes[yuvPlane][j * avmImage.stride[yuvPlane]];
+                    uint8_t * dstRow = &avmImage.planes[yuvPlane][(size_t)j * avmImage.stride[yuvPlane]];
                     memcpy(dstRow, srcRow, bytesPerRow);
                 }
             }
diff --git a/src/codec_svt.c b/src/codec_svt.c
index 353bec48..2b887e85 100644
--- a/src/codec_svt.c
+++ b/src/codec_svt.c
@@ -274,26 +274,21 @@ static avifResult svtCodecEncodeImage(avifCodec * codec,
     if (alpha) {
         input_picture_buffer->y_stride = image->alphaRowBytes / bytesPerPixel;
         input_picture_buffer->luma = image->alphaPlane;
-        input_buffer->n_filled_len = (uint32_t)((size_t)image->alphaRowBytes * image->height);
+        const size_t alphaSize = (size_t)image->alphaRowBytes * image->height;
+        if (alphaSize > UINT32_MAX) {
+            goto cleanup;
+        }
+        input_buffer->n_filled_len = (uint32_t)alphaSize;
 
 #if SVT_AV1_CHECK_VERSION(1, 8, 0)
         // Simulate 4:2:0 UV planes. SVT-AV1 does not support 4:0:0 samples.
-        const size_t uvWidth = ((size_t)image->width + y_shift) >> y_shift;
-
-        // Use size_t to avoid 32-bit overflow
-        const size_t uvRowBytes = (size_t)uvWidth * (size_t)bytesPerPixel;
-
-        // Verify multiplication overflow
-        if (uvWidth != 0 &&
-            uvRowBytes / (size_t)uvWidth != (size_t)bytesPerPixel) {
+        const uint32_t uvWidth = (image->width + y_shift) >> y_shift;
+        const uint32_t uvRowBytes = uvWidth * bytesPerPixel;
+        const size_t uvSize = (size_t)uvRowBytes * uvHeight;
+        if (uvSize > UINT32_MAX / 2) {
             goto cleanup;
         }
-
-        const size_t uvSize = uvRowBytes * (size_t)uvHeight;
-
-        // Verify second multiplication overflow
-        if (uvHeight != 0 &&
-            uvSize / (size_t)uvHeight != uvRowBytes) {
+        if (uvSize * 2 > UINT32_MAX - input_buffer->n_filled_len) {
             goto cleanup;
         }
         uvPlanes = avifAlloc(uvSize);
@@ -305,8 +300,8 @@ static avifResult svtCodecEncodeImage(avifCodec * codec,
         input_buffer->n_filled_len += (uint32_t)uvSize;
         input_picture_buffer->cr = uvPlanes;
         input_buffer->n_filled_len += (uint32_t)uvSize;
-        input_picture_buffer->cb_stride = (uint32_t)uvWidth;
-        input_picture_buffer->cr_stride = (uint32_t)uvWidth;
+        input_picture_buffer->cb_stride = uvWidth;
+        input_picture_buffer->cr_stride = uvWidth;
 #else
         // This workaround was not needed before SVT-AV1 1.8.0.
         // See https://github.com/AOMediaCodec/libavif/issues/1992.
@@ -315,11 +310,23 @@ static avifResult svtCodecEncodeImage(avifCodec * codec,
     } else {
         input_picture_buffer->y_stride = image->yuvRowBytes[0] / bytesPerPixel;
         input_picture_buffer->luma = image->yuvPlanes[0];
-        input_buffer->n_filled_len = (uint32_t)((size_t)image->yuvRowBytes[0] * image->height);
+        const size_t ySize = (size_t)image->yuvRowBytes[0] * image->height;
+        if (ySize > UINT32_MAX) {
+            goto cleanup;
+        }
+        input_buffer->n_filled_len = (uint32_t)ySize;
         input_picture_buffer->cb = image->yuvPlanes[1];
-        input_buffer->n_filled_len += (uint32_t)((size_t)image->yuvRowBytes[1] * uvHeight);
+        const size_t uSize = (size_t)image->yuvRowBytes[1] * uvHeight;
+        if (uSize > UINT32_MAX - input_buffer->n_filled_len) {
+            goto cleanup;
+        }
+        input_buffer->n_filled_len += (uint32_t)uSize;
         input_picture_buffer->cr = image->yuvPlanes[2];
-        input_buffer->n_filled_len += (uint32_t)((size_t)image->yuvRowBytes[2] * uvHeight);
+        const size_t vSize = (size_t)image->yuvRowBytes[2] * uvHeight;
+        if (vSize > UINT32_MAX - input_buffer->n_filled_len) {
+            goto cleanup;
+        }
+        input_buffer->n_filled_len += (uint32_t)vSize;
         input_picture_buffer->cb_stride = image->yuvRowBytes[1] / bytesPerPixel;
         input_picture_buffer->cr_stride = image->yuvRowBytes[2] / bytesPerPixel;
     }
diff --git a/src/reformat.c b/src/reformat.c
index 3f89c6f4..ec16bdf7 100644
--- a/src/reformat.c
+++ b/src/reformat.c
@@ -1854,8 +1854,7 @@ void avifGetRGBAPixel(const avifRGBImage * src, uint32_t x, uint32_t y, const av
     assert(!src->isFloat || src->depth == 16);
     assert(src->format != AVIF_RGB_FORMAT_RGB_565 || src->depth == 8);
 
-    const size_t offset = (size_t)y * (size_t)src->rowBytes + (size_t)x * (size_t)info->pixelBytes;
-    const uint8_t * const srcPixel = &src->pixels[offset];
+    const uint8_t * const srcPixel = &src->pixels[(size_t)y * src->rowBytes + (size_t)x * info->pixelBytes];
     if (info->channelBytes > 1) {
         uint16_t r = *((const uint16_t *)(&srcPixel[info->offsetBytesR]));
         uint16_t g = *((const uint16_t *)(&srcPixel[info->offsetBytesG]));
@@ -1898,7 +1897,7 @@ void avifSetRGBAPixel(const avifRGBImage * dst, uint32_t x, uint32_t y, const av
     assert(rgbaPixel[1] >= 0.0f && rgbaPixel[1] <= 1.0f);
     assert(rgbaPixel[2] >= 0.0f && rgbaPixel[2] <= 1.0f);
 
-    uint8_t * const dstPixel = &dst->pixels[(size_t)y * dst->rowBytes + x * info->pixelBytes];
+    uint8_t * const dstPixel = &dst->pixels[(size_t)y * dst->rowBytes + (size_t)x * info->pixelBytes];
 
     uint8_t * const ptrR = &dstPixel[info->offsetBytesR];
     uint8_t * const ptrG = &dstPixel[info->offsetBytesG];

[rootvector2]

Done

[wantehchang]

Dexter.k: For future reference: This for loop could be also be rewritten as follows, which avoids multiplying rowBytes / stride with the loop index j:

            const uint8_t * srcAlphaRow = image->alphaPlane;
            uint8_t * dstAlphaRow = aomImage.planes[0];
            for (uint32_t j = 0; j < image->height; ++j) {
                memcpy(dstAlphaRow, srcAlphaRow, bytesPerRow);
                srcAlphaRow += image->alphaRowBytes;
                dstAlphaRow += aomImage.stride[0];
            }

[wantehchang]

Dexter.k: Please undo the changes to these four lines. This function uses signed integers (which may be negative) and unsigned integers in some expressions, which is not ideal. We need to modify this function carefully.

diff --git a/src/reformat.c b/src/reformat.c
index ec16bdf7..7b53addf 100644
--- a/src/reformat.c
+++ b/src/reformat.c
@@ -664,10 +664,10 @@ static avifResult avifImageYUVAnyToRGBAnySlow(const avifImage * image,
     const uint8_t * uPlane = image->yuvPlanes[AVIF_CHAN_U];
     const uint8_t * vPlane = image->yuvPlanes[AVIF_CHAN_V];
     const uint8_t * aPlane = image->alphaPlane;
-    const size_t yRowBytes = image->yuvRowBytes[AVIF_CHAN_Y];
-    const size_t uRowBytes = image->yuvRowBytes[AVIF_CHAN_U];
-    const size_t vRowBytes = image->yuvRowBytes[AVIF_CHAN_V];
-    const size_t aRowBytes = image->alphaRowBytes;
+    const uint32_t yRowBytes = image->yuvRowBytes[AVIF_CHAN_Y];
+    const uint32_t uRowBytes = image->yuvRowBytes[AVIF_CHAN_U];
+    const uint32_t vRowBytes = image->yuvRowBytes[AVIF_CHAN_V];
+    const uint32_t aRowBytes = image->alphaRowBytes;
 
     // Various observations and limits
     const avifBool yuvHasColor = (uPlane && vPlane && (image->yuvFormat != AVIF_PIXEL_FORMAT_YUV400));

[rootvector2]

thanks for the review I've applied your patch and reverted the size_t changes to the four rowBytes variables in avifImageYUVAnyToRGBAnySlow back to uint32_t. Let me know if anything else needs to be adjusted.

[wantehchang]

LGTM. Thanks!

[rootvector2]

@wantehchang
Thanks a lot for the review. If you have any advice on what I should focus on next or areas in the codebase that need attention, I’d really appreciate it.

[wantehchang]

Dexter.k: Recall that I asked you to revert your changes to these four rowBytes variables. So we still need to audit this function (avifImageYUVAnyToRGBAnySlow). Please treat expressions involving the following four variables carefully:

                    int uAdjCol, vAdjCol, uAdjRow, vAdjRow;

Note that they are of the signed int type because they may be negative. Right now they are used in expressions consisting of uint32_t and int variables, for example,

                        unormU[0][1] = uPlane[(uvJ * uRowBytes) + (uvI * yuvChannelBytes) + uAdjRow];

So one needs to be familiar with C's "usual arithmetic conversion" to know how those expressions are evaluated. I think it would be clearer if those expressions only use signed variables. I suspect we could use the ptrdiff_t type in those expressions.

As you know, there are several techniques to avoid integer overflows in multiplications involving rowBytes:

• casting a variable to size_t,
• declaring a size_t local variable, or
• replacing multiplication with buffer pointer increment in for loops.

You may need to experiment with them to see which one works the best.

[rootvector2]

ok I am on it

[rootvector2]

@wantehchang
can you check this i have done it
#3063