Fix memory leak in avifImageSplitGrid() on partial initialization failure by nidu-ninja · Pull Request #3092 · AOMediaCodec/libavif

nidu-ninja · 2026-03-10T14:59:09Z

This patch fixes a memory leak in avifImageSplitGrid() that occurs when the function exits early due to an error after allocating one or more grid cells.

The function allocates grid cell images incrementally inside nested loops and performs several operations that may fail (such as avifImageSetViewRect(), avifGainMapCreate(), or metadata copy operations). If any of these steps fail, the function previously returned immediately without destroying already allocated cells.

This patch introduces centralized cleanup logic that ensures all previously created grid cells are destroyed before returning on failure.

The change preserves the existing API and behavior while ensuring correct resource cleanup.

…lure

wantehchang · 2026-03-10T15:55:33Z

apps/shared/avifutil.c

+                gridCells[i] = NULL;
+            }
+        }
+    }


NiDU-NINJA: Thank you for the patch. I looked into this issue. I found that the callers of avifImageSplitGrid() are responsible for destroying the images in the gridCells array if avifImageSplitGrid() fails. So this patch is not needed.

The comment for avifImageSplitGrid() says:

// Splits an image into a grid of cells, including its gain map, if any. // The returned cells must be destroyed with avifImageDestroy().

We can clarify the second sentence to note that it also applies when the function fails.

NiDU-NINJA: I just realized that having the function destroy the images in the gridCells array before returning AVIF_FALSE is overall a win because we can delete the corresponding code in two callers. The new behavior is also more intuitive because the function doesn't return any cells on failure.

Please edit the commit message because there is no memory leak in the current code.

Please apply the following patch:

diff --git a/apps/avifgainmaputil/imageio.cc b/apps/avifgainmaputil/imageio.cc index 9a7e4e88..d133e231 100644 --- a/apps/avifgainmaputil/imageio.cc +++ b/apps/avifgainmaputil/imageio.cc @@ -105,11 +105,6 @@ avifResult WriteAvifGrid(const avifImage* image, int grid_cols, int grid_rows, std::vector<avifImage*> grid_cells_ptrs(grid_cell_count); if (!avifImageSplitGrid(image, grid_cols, grid_rows, grid_cells_ptrs.data())) { - for (avifImage* img : grid_cells_ptrs) { - if (img) { - avifImageDestroy(img); - } - } return AVIF_RESULT_UNKNOWN_ERROR; } // Take ownership of the pointers returned by avifImageSplitGrid. diff --git a/apps/shared/avifutil.c b/apps/shared/avifutil.c index 702c95d4..d2828a46 100644 --- a/apps/shared/avifutil.c +++ b/apps/shared/avifutil.c @@ -598,11 +598,6 @@ avifBool avifImageSplitGrid(const avifImage * gridSplitImage, uint32_t gridCols, goto cleanup; } if (!avifImageSplitGrid(gridSplitImage->gainMap->image, gridCols, gridRows, gainMapGridCells)) { - for (uint32_t i = 0; i < gridCols * gridRows; ++i) { - if (gainMapGridCells[i]) { - avifImageDestroy(gainMapGridCells[i]); - } - } free(gainMapGridCells); goto cleanup; } @@ -655,10 +650,8 @@ avifBool avifImageSplitGrid(const avifImage * gridSplitImage, uint32_t gridCols, cleanup: if (!success) { for (uint32_t i = 0; i < createdCells; ++i) { - if (gridCells && gridCells[i]) { - avifImageDestroy(gridCells[i]); - gridCells[i] = NULL; - } + avifImageDestroy(gridCells[i]); + gridCells[i] = NULL; } } return success; @@ -821,4 +814,4 @@ avifResult avifApplyTransforms(avifRGBImage * dstView, avifRGBImage * srcImage, } } return AVIF_RESULT_OK; -} \ No newline at end of file +}

wantehchang · 2026-03-11T01:33:57Z

apps/shared/avifutil.c

+                gridCells[i] = NULL;
+            }
+        }
+    }


NiDU-NINJA: I just realized that having the function destroy the images in the gridCells array before returning AVIF_FALSE is overall a win because we can delete the corresponding code in two callers. The new behavior is also more intuitive because the function doesn't return any cells on failure.

Please edit the commit message because there is no memory leak in the current code.

Please apply the following patch:

diff --git a/apps/avifgainmaputil/imageio.cc b/apps/avifgainmaputil/imageio.cc index 9a7e4e88..d133e231 100644 --- a/apps/avifgainmaputil/imageio.cc +++ b/apps/avifgainmaputil/imageio.cc @@ -105,11 +105,6 @@ avifResult WriteAvifGrid(const avifImage* image, int grid_cols, int grid_rows, std::vector<avifImage*> grid_cells_ptrs(grid_cell_count); if (!avifImageSplitGrid(image, grid_cols, grid_rows, grid_cells_ptrs.data())) { - for (avifImage* img : grid_cells_ptrs) { - if (img) { - avifImageDestroy(img); - } - } return AVIF_RESULT_UNKNOWN_ERROR; } // Take ownership of the pointers returned by avifImageSplitGrid. diff --git a/apps/shared/avifutil.c b/apps/shared/avifutil.c index 702c95d4..d2828a46 100644 --- a/apps/shared/avifutil.c +++ b/apps/shared/avifutil.c @@ -598,11 +598,6 @@ avifBool avifImageSplitGrid(const avifImage * gridSplitImage, uint32_t gridCols, goto cleanup; } if (!avifImageSplitGrid(gridSplitImage->gainMap->image, gridCols, gridRows, gainMapGridCells)) { - for (uint32_t i = 0; i < gridCols * gridRows; ++i) { - if (gainMapGridCells[i]) { - avifImageDestroy(gainMapGridCells[i]); - } - } free(gainMapGridCells); goto cleanup; } @@ -655,10 +650,8 @@ avifBool avifImageSplitGrid(const avifImage * gridSplitImage, uint32_t gridCols, cleanup: if (!success) { for (uint32_t i = 0; i < createdCells; ++i) { - if (gridCells && gridCells[i]) { - avifImageDestroy(gridCells[i]); - gridCells[i] = NULL; - } + avifImageDestroy(gridCells[i]); + gridCells[i] = NULL; } } return success; @@ -821,4 +814,4 @@ avifResult avifApplyTransforms(avifRGBImage * dstView, avifRGBImage * srcImage, } } return AVIF_RESULT_OK; -} \ No newline at end of file +}

wantehchang · 2026-03-11T01:35:15Z

apps/shared/avifutil.c

+cleanup:
+    if (!success) {
+        for (uint32_t i = 0; i < createdCells; ++i) {
+            if (gridCells && gridCells[i]) {


This check is not needed. (See patch below.)

This function assumes gridCells is not null. This is the API contract.

gridCells[i] cannot possibly be null.

apps/shared/avifutil.c

…cleanup

nidu-ninja · 2026-03-16T20:23:48Z

@wantehchang i have done the changes as you said

wantehchang · 2026-03-27T00:50:23Z

apps/shared/avifutil.c

    }
    const avifBool hasGainMap = gridSplitImage->gainMap && gridSplitImage->gainMap->image;

+    uint32_t createdCells = 0;


I moved the declaration of createdCells right before the nested for loops that increment it.

wantehchang · 2026-03-27T00:50:58Z

apps/shared/avifutil.c

+                goto cleanup;
            }
            gridCells[gridIndex] = cellImage;
+            assert(gridIndex == createdCells);


I added an assertion here to verify that gridIndex is consecutive. The cleanup code depends on this property.

apps/shared/avifutil.c

After pull request AOMediaCodec#3092, avifImageSplitGrid() doesn't return any grid cells on failure. So only need to destroy returned cells on success.

After pull request #3092, avifImageSplitGrid() doesn't return any grid cells on failure. So only need to destroy returned cells on success.

Fix memory leak in avifImageSplitGrid() on partial initialization fai…

e14065b

…lure

wantehchang reviewed Mar 10, 2026

View reviewed changes

wantehchang reviewed Mar 11, 2026

View reviewed changes

Simplify error handling in avifImageSplitGrid() and remove redundant …

833e09f

…cleanup

Move variable declarations, add an assertion

c02ecad

wantehchang merged commit 35f24ce into AOMediaCodec:main Mar 27, 2026
25 checks passed

wantehchang reviewed Mar 27, 2026

View reviewed changes

wantehchang mentioned this pull request Mar 27, 2026

Update avifImageSplitGrid() comment after PR #3092 #3141

Merged

wantehchang added a commit that referenced this pull request Mar 27, 2026

Update avifImageSplitGrid() comment after PR #3092

83373d5

After pull request #3092, avifImageSplitGrid() doesn't return any grid cells on failure. So only need to destroy returned cells on success.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak in avifImageSplitGrid() on partial initialization failure#3092

Fix memory leak in avifImageSplitGrid() on partial initialization failure#3092
wantehchang merged 3 commits intoAOMediaCodec:mainfrom
nidu-ninja:avifImageSplitGrid-cleanup-on-failure

nidu-ninja commented Mar 10, 2026

Uh oh!

wantehchang Mar 10, 2026

Uh oh!

wantehchang Mar 11, 2026

Uh oh!

wantehchang Mar 11, 2026

Uh oh!

wantehchang Mar 11, 2026

Uh oh!

Uh oh!

nidu-ninja commented Mar 16, 2026

Uh oh!

Uh oh!

wantehchang Mar 27, 2026

Uh oh!

wantehchang Mar 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

nidu-ninja commented Mar 10, 2026

Uh oh!

wantehchang Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

wantehchang Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

wantehchang Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

wantehchang Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nidu-ninja commented Mar 16, 2026

Uh oh!

Uh oh!

wantehchang Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

wantehchang Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants