Skip to content

Commit b6fb186

Browse files
Fix memory leaks on error in ComputeGainMap() (#3336)
The "Unsupported RGB color space" path and the avifImageScale() failure path returned directly instead of going through the cleanup label, leaking the gainMapF[] channel buffers and the gainMapRGB pixels. Both now set res and jump to cleanup, as documented by the comment above the allocations. The regression test computes a gain map with an unsupported depth (only rejected after the allocations) and checks that the gain map image planes are freed on error. Verified with ASan/LSan. Co-authored-by: krishna28238-arch <319297638+krishna28238-arch@users.noreply.github.com>
1 parent 115bc52 commit b6fb186

2 files changed

Lines changed: 32 additions & 2 deletions

File tree

src/gainmap.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -804,7 +804,8 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage,
804804
avifRGBColorSpaceInfo gainMapRGBInfo;
805805
if (!avifGetRGBColorSpaceInfo(&gainMapRGB, &gainMapRGBInfo)) {
806806
avifDiagnosticsPrintf(diag, "Unsupported RGB color space");
807-
return AVIF_RESULT_NOT_IMPLEMENTED;
807+
res = AVIF_RESULT_NOT_IMPLEMENTED;
808+
goto cleanup;
808809
}
809810
for (uint32_t j = 0; j < height; ++j) {
810811
for (uint32_t i = 0; i < width; ++i) {
@@ -825,7 +826,10 @@ avifResult avifRGBImageComputeGainMap(const avifRGBImage * baseRgbImage,
825826
// Scale down the gain map if requested.
826827
// Another way would be to scale the source images, but it seems to perform worse.
827828
if (requestedWidth != gainMapImage->width || requestedHeight != gainMapImage->height) {
828-
AVIF_CHECKRES(avifImageScale(gainMap->image, requestedWidth, requestedHeight, diag));
829+
res = avifImageScale(gainMap->image, requestedWidth, requestedHeight, diag);
830+
if (res != AVIF_RESULT_OK) {
831+
goto cleanup;
832+
}
829833
}
830834

831835
cleanup:

tests/gtest/avifgainmaptest.cc

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1593,6 +1593,32 @@ TEST(GainMapTest, CreateGainMapConstantFactor) {
15931593
<< avifResultToString(result) << " " << diag.error;
15941594
}
15951595

1596+
// avifRGBImageComputeGainMap() should free all its intermediate buffers when
1597+
// it fails, in particular when the gain map depth is only rejected after the
1598+
// allocations happened.
1599+
TEST(GainMapTest, ComputeGainMapUnsupportedDepth) {
1600+
// Used only to initialize rgb images.
1601+
ImagePtr yuv(avifImageCreate(10, 10, 8, AVIF_PIXEL_FORMAT_YUV444));
1602+
testutil::AvifRgbImage base_image(yuv.get(), 8, AVIF_RGB_FORMAT_RGB);
1603+
testutil::AvifRgbImage alt_image(yuv.get(), 8, AVIF_RGB_FORMAT_RGB);
1604+
for (uint32_t i = 0; i < base_image.width * base_image.height * 3; ++i) {
1605+
base_image.pixels[i] = 10;
1606+
alt_image.pixels[i] = 200;
1607+
}
1608+
GainMapPtr gain_map(avifGainMapCreate());
1609+
gain_map->image = avifImageCreate(5, 5, 9, AVIF_PIXEL_FORMAT_YUV444);
1610+
avifDiagnostics diag;
1611+
avifResult result = avifRGBImageComputeGainMap(
1612+
&base_image, AVIF_COLOR_PRIMARIES_SRGB,
1613+
AVIF_TRANSFER_CHARACTERISTICS_SRGB, &alt_image, AVIF_COLOR_PRIMARIES_SRGB,
1614+
AVIF_TRANSFER_CHARACTERISTICS_SRGB, gain_map.get(), &diag);
1615+
1616+
EXPECT_EQ(result, AVIF_RESULT_NOT_IMPLEMENTED)
1617+
<< avifResultToString(result) << " " << diag.error;
1618+
// The planes are freed on error, per the cleanup contract.
1619+
EXPECT_EQ(gain_map->image->yuvPlanes[0], nullptr);
1620+
}
1621+
15961622
TEST(FindMinMaxWithoutOutliers, AllSame) {
15971623
constexpr int kNumValues = 10000;
15981624

0 commit comments

Comments
 (0)