Make avifImageAllocatePlanes() return avifResult

y-guyon · web-flow · commit 1ce971ee6de4 · 2022-08-01T20:22:04.000+02:00
To catch memory allocation failures and invalid parameters, it is
safer to return a status from avifImageAllocatePlanes() and from
avifImageCopy().
Update CHANGELOG.md.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,12 +6,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-There is an incompatible ABI change in this release. Members were removed from
-avifImage struct. It is necessary to recompile your code.
+There are incompatible ABI changes in this release. The alphaRange member was
+removed from avifImage struct. avifImageCopy() and avifImageAllocatePlanes()
+signatures changed. It is necessary to recompile your code. Also check the
+return values of avifImageCopy() and avifImageAllocatePlanes().
 
 ### Changed
 * Update aom.cmd: v3.4.0
 * Update svt.cmd/svt.sh: v1.1.0
+* avifImageCopy() and avifImageAllocatePlanes() now return avifResult instead of
+  void to report invalid parameters or memory allocation failures
 
 ### Removed
 * alphaRange field was removed from the avifImage struct. It it presumed that
diff --git a/apps/avifenc.c b/apps/avifenc.c
@@ -352,7 +352,11 @@ static avifBool avifImageSplitGrid(const avifImage * gridSplitImage, uint32_t gr
             avifImage * cellImage = avifImageCreateEmpty();
             gridCells[gridIndex] = cellImage;
 
-            avifImageCopy(cellImage, gridSplitImage, 0);
+            const avifResult copyResult = avifImageCopy(cellImage, gridSplitImage, 0);
+            if (copyResult != AVIF_RESULT_OK) {
+                fprintf(stderr, "ERROR: Image copy failed: %s\n", avifResultToString(copyResult));
+                return AVIF_FALSE;
+            }
             cellImage->width = cellWidth;
             cellImage->height = cellHeight;
 
diff --git a/apps/shared/avifjpeg.c b/apps/shared/avifjpeg.c
@@ -40,7 +40,7 @@ static void my_error_exit(j_common_ptr cinfo)
 
 // An internal function used by avifJPEGReadCopy(), this is the shared libjpeg decompression code
 // for all paths avifJPEGReadCopy() takes.
-static void avifJPEGCopyPixels(avifImage * avif, struct jpeg_decompress_struct * cinfo)
+static avifBool avifJPEGCopyPixels(avifImage * avif, struct jpeg_decompress_struct * cinfo)
 {
     cinfo->raw_data_out = TRUE;
     jpeg_start_decompress(cinfo);
@@ -68,7 +68,9 @@ static void avifJPEGCopyPixels(avifImage * avif, struct jpeg_decompress_struct *
         readLines = AVIF_MAX(readLines, linesPerCall[i]);
     }
 
-    avifImageAllocatePlanes(avif, AVIF_PLANES_YUV);
+    if (avifImageAllocatePlanes(avif, AVIF_PLANES_YUV) != AVIF_RESULT_OK) {
+        return AVIF_FALSE;
+    }
 
     // destination avif channel for each jpeg channel
     avifChannelIndex targetChannel[3] = { AVIF_CHAN_Y, AVIF_CHAN_Y, AVIF_CHAN_Y };
@@ -102,6 +104,7 @@ static void avifJPEGCopyPixels(avifImage * avif, struct jpeg_decompress_struct *
             alreadyRead[i] += linesPerCall[i];
         }
     }
+    return AVIF_TRUE;
 }
 
 static avifBool avifJPEGHasCompatibleMatrixCoefficients(avifMatrixCoefficients matrixCoefficients)
@@ -148,19 +151,15 @@ static avifBool avifJPEGReadCopy(avifImage * avif, struct jpeg_decompress_struct
                 }
                 if (avif->yuvFormat == jpegFormat) {
                     cinfo->out_color_space = JCS_YCbCr;
-                    avifJPEGCopyPixels(avif, cinfo);
-
-                    return AVIF_TRUE;
+                    return avifJPEGCopyPixels(avif, cinfo);
                 }
             }
 
             // YUV->Grayscale: subsample Y plane not allowed.
             if ((avif->yuvFormat == AVIF_PIXEL_FORMAT_YUV400) && (cinfo->comp_info[0].h_samp_factor == cinfo->max_h_samp_factor &&
                                                                   cinfo->comp_info[0].v_samp_factor == cinfo->max_v_samp_factor)) {
                 cinfo->out_color_space = JCS_YCbCr;
-                avifJPEGCopyPixels(avif, cinfo);
-
-                return AVIF_TRUE;
+                return avifJPEGCopyPixels(avif, cinfo);
             }
         }
     } else if (cinfo->jpeg_color_space == JCS_GRAYSCALE) {
@@ -173,16 +172,16 @@ static avifBool avifJPEGReadCopy(avifImage * avif, struct jpeg_decompress_struct
                 if ((avif->yuvFormat == AVIF_PIXEL_FORMAT_YUV400) || (avif->yuvFormat == AVIF_PIXEL_FORMAT_NONE)) {
                     avif->yuvFormat = AVIF_PIXEL_FORMAT_YUV400;
                     cinfo->out_color_space = JCS_GRAYSCALE;
-                    avifJPEGCopyPixels(avif, cinfo);
-
-                    return AVIF_TRUE;
+                    return avifJPEGCopyPixels(avif, cinfo);
                 }
 
                 // Grayscale->YUV: copy Y, fill UV with monochrome value.
                 if ((avif->yuvFormat == AVIF_PIXEL_FORMAT_YUV444) || (avif->yuvFormat == AVIF_PIXEL_FORMAT_YUV422) ||
                     (avif->yuvFormat == AVIF_PIXEL_FORMAT_YUV420)) {
                     cinfo->out_color_space = JCS_GRAYSCALE;
-                    avifJPEGCopyPixels(avif, cinfo);
+                    if (!avifJPEGCopyPixels(avif, cinfo)) {
+                        return AVIF_FALSE;
+                    }
 
                     avifPixelFormatInfo info;
                     avifGetPixelFormatInfo(avif->yuvFormat, &info);
@@ -199,7 +198,9 @@ static avifBool avifJPEGReadCopy(avifImage * avif, struct jpeg_decompress_struct
                 ((avif->yuvFormat == AVIF_PIXEL_FORMAT_YUV444) || (avif->yuvFormat == AVIF_PIXEL_FORMAT_NONE))) {
                 avif->yuvFormat = AVIF_PIXEL_FORMAT_YUV444;
                 cinfo->out_color_space = JCS_GRAYSCALE;
-                avifJPEGCopyPixels(avif, cinfo);
+                if (!avifJPEGCopyPixels(avif, cinfo)) {
+                    return AVIF_FALSE;
+                }
 
                 memcpy(avif->yuvPlanes[AVIF_CHAN_U], avif->yuvPlanes[AVIF_CHAN_Y], (size_t)avif->yuvRowBytes[AVIF_CHAN_U] * avif->height);
                 memcpy(avif->yuvPlanes[AVIF_CHAN_V], avif->yuvPlanes[AVIF_CHAN_Y], (size_t)avif->yuvRowBytes[AVIF_CHAN_V] * avif->height);
@@ -216,9 +217,7 @@ static avifBool avifJPEGReadCopy(avifImage * avif, struct jpeg_decompress_struct
              cinfo->comp_info[2].h_samp_factor == 1 && cinfo->comp_info[2].v_samp_factor == 1)) {
             avif->yuvFormat = AVIF_PIXEL_FORMAT_YUV444;
             cinfo->out_color_space = JCS_RGB;
-            avifJPEGCopyPixels(avif, cinfo);
-
-            return AVIF_TRUE;
+            return avifJPEGCopyPixels(avif, cinfo);
         }
     }
 
diff --git a/apps/shared/y4m.c b/apps/shared/y4m.c
@@ -348,16 +348,17 @@ avifBool y4mRead(const char * inputFilename, avifImage * avif, avifAppSourceTimi
         *sourceTiming = frame.sourceTiming;
     }
 
-    avifImageFreePlanes(avif, AVIF_PLANES_YUV | AVIF_PLANES_A);
+    avifImageFreePlanes(avif, AVIF_PLANES_ALL);
     avif->width = frame.width;
     avif->height = frame.height;
     avif->depth = frame.depth;
     avif->yuvFormat = frame.format;
     avif->yuvRange = frame.range;
     avif->yuvChromaSamplePosition = frame.chromaSamplePosition;
-    avifImageAllocatePlanes(avif, AVIF_PLANES_YUV);
-    if (frame.hasAlpha) {
-        avifImageAllocatePlanes(avif, AVIF_PLANES_A);
+    avifResult allocationResult = avifImageAllocatePlanes(avif, frame.hasAlpha ? AVIF_PLANES_ALL : AVIF_PLANES_YUV);
+    if (allocationResult != AVIF_RESULT_OK) {
+        fprintf(stderr, "Failed to allocate the planes: %s\n", avifResultToString(allocationResult));
+        goto cleanup;
     }
 
     avifPixelFormatInfo formatInfo;
diff --git a/examples/avif_example_encode.c b/examples/avif_example_encode.c
@@ -41,7 +41,11 @@ int main(int argc, char * argv[])
         // If you have YUV(A) data you want to encode, use this path
         printf("Encoding raw YUVA data\n");
 
-        avifImageAllocatePlanes(image, AVIF_PLANES_YUV | AVIF_PLANES_A);
+        const avifResult allocateResult = avifImageAllocatePlanes(image, AVIF_PLANES_ALL);
+        if (allocateResult != AVIF_RESULT_OK) {
+            fprintf(stderr, "Failed to allocate the planes: %s\n", avifResultToString(allocateResult));
+            goto cleanup;
+        }
 
         // Fill your YUV(A) data here
         memset(image->yuvPlanes[AVIF_CHAN_Y], 255, image->yuvRowBytes[AVIF_CHAN_Y] * image->height);
diff --git a/include/avif/avif.h b/include/avif/avif.h
@@ -466,7 +466,7 @@ typedef struct avifImage
 
 AVIF_API avifImage * avifImageCreate(int width, int height, int depth, avifPixelFormat yuvFormat);
 AVIF_API avifImage * avifImageCreateEmpty(void); // helper for making an image to decode into
-AVIF_API void avifImageCopy(avifImage * dstImage, const avifImage * srcImage, avifPlanesFlags planes); // deep copy
+AVIF_API avifResult avifImageCopy(avifImage * dstImage, const avifImage * srcImage, avifPlanesFlags planes); // deep copy
 AVIF_API avifResult avifImageSetViewRect(avifImage * dstImage, const avifImage * srcImage, const avifCropRect * rect); // shallow copy, no metadata
 AVIF_API void avifImageDestroy(avifImage * image);
 
@@ -476,8 +476,8 @@ AVIF_API void avifImageSetProfileICC(avifImage * image, const uint8_t * icc, siz
 AVIF_API void avifImageSetMetadataExif(avifImage * image, const uint8_t * exif, size_t exifSize);
 AVIF_API void avifImageSetMetadataXMP(avifImage * image, const uint8_t * xmp, size_t xmpSize);
 
-AVIF_API void avifImageAllocatePlanes(avifImage * image, avifPlanesFlags planes); // Ignores any pre-existing planes
-AVIF_API void avifImageFreePlanes(avifImage * image, avifPlanesFlags planes);     // Ignores already-freed planes
+AVIF_API avifResult avifImageAllocatePlanes(avifImage * image, avifPlanesFlags planes); // Ignores any pre-existing planes
+AVIF_API void avifImageFreePlanes(avifImage * image, avifPlanesFlags planes);           // Ignores already-freed planes
 AVIF_API void avifImageStealPlanes(avifImage * dstImage, avifImage * srcImage, avifPlanesFlags planes);
 
 // ---------------------------------------------------------------------------
diff --git a/src/avif.c b/src/avif.c
@@ -3,6 +3,8 @@
 
 #include "avif/internal.h"
 
+#include <limits.h>
+#include <stdint.h>
 #include <string.h>
 
 #define STR_HELPER(x) #x
@@ -163,7 +165,7 @@ static void avifImageCopyNoAlloc(avifImage * dstImage, const avifImage * srcImag
     dstImage->imir = srcImage->imir;
 }
 
-void avifImageCopy(avifImage * dstImage, const avifImage * srcImage, avifPlanesFlags planes)
+avifResult avifImageCopy(avifImage * dstImage, const avifImage * srcImage, avifPlanesFlags planes)
 {
     avifImageFreePlanes(dstImage, AVIF_PLANES_ALL);
     avifImageCopyNoAlloc(dstImage, srcImage);
@@ -174,7 +176,10 @@ void avifImageCopy(avifImage * dstImage, const avifImage * srcImage, avifPlanesF
     avifImageSetMetadataXMP(dstImage, srcImage->xmp.data, srcImage->xmp.size);
 
     if ((planes & AVIF_PLANES_YUV) && srcImage->yuvPlanes[AVIF_CHAN_Y]) {
-        avifImageAllocatePlanes(dstImage, AVIF_PLANES_YUV);
+        const avifResult allocationResult = avifImageAllocatePlanes(dstImage, AVIF_PLANES_YUV);
+        if (allocationResult != AVIF_RESULT_OK) {
+            return allocationResult;
+        }
 
         avifPixelFormatInfo formatInfo;
         avifGetPixelFormatInfo(srcImage->yuvFormat, &formatInfo);
@@ -200,13 +205,17 @@ void avifImageCopy(avifImage * dstImage, const avifImage * srcImage, avifPlanesF
     }
 
     if ((planes & AVIF_PLANES_A) && srcImage->alphaPlane) {
-        avifImageAllocatePlanes(dstImage, AVIF_PLANES_A);
+        const avifResult allocationResult = avifImageAllocatePlanes(dstImage, AVIF_PLANES_A);
+        if (allocationResult != AVIF_RESULT_OK) {
+            return allocationResult;
+        }
         for (uint32_t j = 0; j < dstImage->height; ++j) {
             uint8_t * srcAlphaRow = &srcImage->alphaPlane[j * srcImage->alphaRowBytes];
             uint8_t * dstAlphaRow = &dstImage->alphaPlane[j * dstImage->alphaRowBytes];
             memcpy(dstAlphaRow, srcAlphaRow, dstImage->alphaRowBytes);
         }
     }
+    return AVIF_RESULT_OK;
 }
 
 avifResult avifImageSetViewRect(avifImage * dstImage, const avifImage * srcImage, const avifCropRect * rect)
@@ -264,45 +273,70 @@ void avifImageSetMetadataXMP(avifImage * image, const uint8_t * xmp, size_t xmpS
     avifRWDataSet(&image->xmp, xmp, xmpSize);
 }
 
-void avifImageAllocatePlanes(avifImage * image, avifPlanesFlags planes)
+avifResult avifImageAllocatePlanes(avifImage * image, avifPlanesFlags planes)
 {
-    int channelSize = avifImageUsesU16(image) ? 2 : 1;
-    int fullRowBytes = channelSize * image->width;
-    int fullSize = fullRowBytes * image->height;
+    if (image->width == 0 || image->height == 0) {
+        return AVIF_RESULT_INVALID_ARGUMENT;
+    }
+    const size_t channelSize = avifImageUsesU16(image) ? 2 : 1;
+    if (image->width > SIZE_MAX / channelSize) {
+        return AVIF_RESULT_INVALID_ARGUMENT;
+    }
+    const size_t fullRowBytes = channelSize * image->width;
+    if ((fullRowBytes > UINT32_MAX) || (image->height > SIZE_MAX / fullRowBytes)) {
+        return AVIF_RESULT_INVALID_ARGUMENT;
+    }
+    const size_t fullSize = fullRowBytes * image->height;
+
     if ((planes & AVIF_PLANES_YUV) && (image->yuvFormat != AVIF_PIXEL_FORMAT_NONE)) {
         avifPixelFormatInfo info;
         avifGetPixelFormatInfo(image->yuvFormat, &info);
 
-        int shiftedW = (image->width + info.chromaShiftX) >> info.chromaShiftX;
-        int shiftedH = (image->height + info.chromaShiftY) >> info.chromaShiftY;
+        // Intermediary computation as 64 bits in case width or height is exactly UINT32_MAX.
+        const uint32_t shiftedW = (uint32_t)(((uint64_t)image->width + info.chromaShiftX) >> info.chromaShiftX);
+        const uint32_t shiftedH = (uint32_t)(((uint64_t)image->height + info.chromaShiftY) >> info.chromaShiftY);
 
-        int uvRowBytes = channelSize * shiftedW;
-        int uvSize = uvRowBytes * shiftedH;
+        // These are less than or equal to fullRowBytes/fullSize. No need to check overflows.
+        const size_t uvRowBytes = channelSize * shiftedW;
+        const size_t uvSize = uvRowBytes * shiftedH;
 
+        image->imageOwnsYUVPlanes = AVIF_TRUE;
         if (!image->yuvPlanes[AVIF_CHAN_Y]) {
             image->yuvRowBytes[AVIF_CHAN_Y] = fullRowBytes;
             image->yuvPlanes[AVIF_CHAN_Y] = avifAlloc(fullSize);
+            if (!image->yuvPlanes[AVIF_CHAN_Y]) {
+                return AVIF_RESULT_OUT_OF_MEMORY;
+            }
         }
 
         if (image->yuvFormat != AVIF_PIXEL_FORMAT_YUV400) {
             if (!image->yuvPlanes[AVIF_CHAN_U]) {
                 image->yuvRowBytes[AVIF_CHAN_U] = uvRowBytes;
                 image->yuvPlanes[AVIF_CHAN_U] = avifAlloc(uvSize);
+                if (!image->yuvPlanes[AVIF_CHAN_U]) {
+                    return AVIF_RESULT_OUT_OF_MEMORY;
+                }
             }
             if (!image->yuvPlanes[AVIF_CHAN_V]) {
                 image->yuvRowBytes[AVIF_CHAN_V] = uvRowBytes;
                 image->yuvPlanes[AVIF_CHAN_V] = avifAlloc(uvSize);
+                if (!image->yuvPlanes[AVIF_CHAN_V]) {
+                    return AVIF_RESULT_OUT_OF_MEMORY;
+                }
             }
         }
-        image->imageOwnsYUVPlanes = AVIF_TRUE;
     }
     if (planes & AVIF_PLANES_A) {
+        image->imageOwnsAlphaPlane = AVIF_TRUE;
         if (!image->alphaPlane) {
             image->alphaRowBytes = fullRowBytes;
             image->alphaPlane = avifAlloc(fullSize);
+            if (!image->alphaPlane) {
+                return AVIF_RESULT_OUT_OF_MEMORY;
+            }
         }
-        image->imageOwnsAlphaPlane = AVIF_TRUE;
     }
+    return AVIF_RESULT_OK;
 }
 
 void avifImageFreePlanes(avifImage * image, avifPlanesFlags planes)
diff --git a/src/read.c b/src/read.c
@@ -1363,7 +1363,10 @@ static avifBool avifDecoderDataFillImageGrid(avifDecoderData * data,
         }
     }
 
-    avifImageAllocatePlanes(dstImage, alpha ? AVIF_PLANES_A : AVIF_PLANES_YUV);
+    if (avifImageAllocatePlanes(dstImage, alpha ? AVIF_PLANES_A : AVIF_PLANES_YUV) != AVIF_RESULT_OK) {
+        avifDiagnosticsPrintf(data->diag, "Image allocation failure");
+        return AVIF_FALSE;
+    }
 
     avifPixelFormatInfo formatInfo;
     avifGetPixelFormatInfo(firstTile->image->yuvFormat, &formatInfo);
@@ -3744,7 +3747,10 @@ static avifResult avifImageLimitedToFullAlpha(avifImage * image)
     // codec's internal frame buffers. Allocate memory for the conversion.
     image->alphaPlane = NULL;
     image->alphaRowBytes = 0;
-    avifImageAllocatePlanes(image, AVIF_PLANES_A);
+    const avifResult allocationResult = avifImageAllocatePlanes(image, AVIF_PLANES_A);
+    if (allocationResult != AVIF_RESULT_OK) {
+        return allocationResult;
+    }
 
     if (image->depth > 8) {
         for (uint32_t j = 0; j < image->height; ++j) {
@@ -4145,8 +4151,7 @@ avifResult avifDecoderRead(avifDecoder * decoder, avifImage * image)
     if (result != AVIF_RESULT_OK) {
         return result;
     }
-    avifImageCopy(image, decoder->image, AVIF_PLANES_ALL);
-    return AVIF_RESULT_OK;
+    return avifImageCopy(image, decoder->image, AVIF_PLANES_ALL);
 }
 
 avifResult avifDecoderReadMemory(avifDecoder * decoder, avifImage * image, const uint8_t * data, size_t size)
diff --git a/src/reformat.c b/src/reformat.c
@@ -193,10 +193,14 @@ avifResult avifImageRGBToYUV(avifImage * image, const avifRGBImage * rgb)
         return AVIF_RESULT_NOT_IMPLEMENTED;
     }
 
+    const avifBool hasAlpha = avifRGBFormatHasAlpha(rgb->format) && !rgb->ignoreAlpha;
+    avifResult allocationResult = avifImageAllocatePlanes(image, hasAlpha ? AVIF_PLANES_ALL : AVIF_PLANES_YUV);
+    if (allocationResult != AVIF_RESULT_OK) {
+        return allocationResult;
+    }
+
     avifAlphaMultiplyMode alphaMode = AVIF_ALPHA_MULTIPLY_MODE_NO_OP;
-    avifImageAllocatePlanes(image, AVIF_PLANES_YUV);
-    if (avifRGBFormatHasAlpha(rgb->format) && !rgb->ignoreAlpha) {
-        avifImageAllocatePlanes(image, AVIF_PLANES_A);
+    if (hasAlpha) {
         if (!rgb->alphaPremultiplied && image->alphaPremultiplied) {
             alphaMode = AVIF_ALPHA_MULTIPLY_MODE_MULTIPLY;
         } else if (rgb->alphaPremultiplied && !image->alphaPremultiplied) {
diff --git a/src/scale.c b/src/scale.c
@@ -89,7 +89,11 @@ avifBool avifImageScale(avifImage * image, uint32_t dstWidth, uint32_t dstHeight
     }
 
     if (srcYUVPlanes[0]) {
-        avifImageAllocatePlanes(image, AVIF_PLANES_YUV);
+        const avifResult allocationResult = avifImageAllocatePlanes(image, AVIF_PLANES_YUV);
+        if (allocationResult != AVIF_RESULT_OK) {
+            avifDiagnosticsPrintf(diag, "Allocation of YUV planes failed: %s", avifResultToString(allocationResult));
+            return AVIF_FALSE;
+        }
 
         avifPixelFormatInfo formatInfo;
         avifGetPixelFormatInfo(image->yuvFormat, &formatInfo);
@@ -132,7 +136,11 @@ avifBool avifImageScale(avifImage * image, uint32_t dstWidth, uint32_t dstHeight
     }
 
     if (srcAlphaPlane) {
-        avifImageAllocatePlanes(image, AVIF_PLANES_A);
+        const avifResult allocationResult = avifImageAllocatePlanes(image, AVIF_PLANES_A);
+        if (allocationResult != AVIF_RESULT_OK) {
+            avifDiagnosticsPrintf(diag, "Allocation of alpha plane failed: %s", avifResultToString(allocationResult));
+            return AVIF_FALSE;
+        }
 
         if (image->depth > 8) {
             uint16_t * const srcPlane = (uint16_t *)srcAlphaPlane;
diff --git a/src/write.c b/src/write.c
@@ -683,7 +683,10 @@ static avifResult avifEncoderAddImageInternal(avifEncoder * encoder,
 
     if (encoder->data->items.count == 0) {
         // Make a copy of the first image's metadata (sans pixels) for future writing/validation
-        avifImageCopy(encoder->data->imageMetadata, firstCell, 0);
+        const avifResult copyResult = avifImageCopy(encoder->data->imageMetadata, firstCell, 0);
+        if (copyResult != AVIF_RESULT_OK) {
+            return copyResult;
+        }
 
         // Prepare all AV1 items
 
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
diff --git a/tests/gtest/avifallocationtest.cc b/tests/gtest/avifallocationtest.cc
diff --git a/tests/gtest/avifgridapitest.cc b/tests/gtest/avifgridapitest.cc
diff --git a/tests/gtest/aviftest_helpers.cc b/tests/gtest/aviftest_helpers.cc
