Skip to content

Fix unchecked integer overflow in encoder UV plane allocation #3233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jortles wants to merge 2 commits into from
+13 −0
Closed

Fix unchecked integer overflow in encoder UV plane allocation #3233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c5b75a
Fix unchecked integer overflow in encoder UV plane allocation
jortles May 29, 2026
08fc365
Address review: guard dimension +1 overflow, use SIZE_MAX
jortles Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/avif.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ static avifResult avifImageCopyProperties(avifImage * dstImage, const avifImage
dstImage->numProperties = 0;

if (srcImage->numProperties != 0) {
AVIF_CHECKERR(srcImage->numProperties < SIZE_MAX / sizeof(srcImage->properties[0]), AVIF_RESULT_INVALID_ARGUMENT);
Copy link
Copy Markdown
Member

@wantehchang wantehchang Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the change to this file. This has been addressed by PR #3210.

dstImage->properties = (avifImageItemProperty *)avifAlloc(srcImage->numProperties * sizeof(srcImage->properties[0]));
AVIF_CHECKERR(dstImage->properties != NULL, AVIF_RESULT_OUT_OF_MEMORY);
memset(dstImage->properties, 0, srcImage->numProperties * sizeof(srcImage->properties[0]));
Expand Down
6 changes: 6 additions & 0 deletions src/codec_avm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,13 +911,19 @@ static avifResult avmCodecEncodeImage(avifCodec * codec,
// monochrome. Manually set UV planes to 0.5.

// avmImage is always 420 when we're monochrome
if (image->width == UINT32_MAX || image->height == UINT32_MAX) {
Copy link
Copy Markdown
Contributor

@y-guyon y-guyon Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe image->width should be cast to size_t instead of compared against UINT32_MAX? It does not matter much for such huge dimensions I guess.

Copy link
Copy Markdown
Member

@wantehchang wantehchang Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should allow UINT32_MAX.

As Yannis suggested, we can cast image->width to a larger type such as uint64_t. (size_t is 32 bits in 32-bit systems.) We use this approach elsewhere.

Another approach is to replace (image->width + 1) >> 1 with image->width / 2 + image->width % 2, or (since image->width >= 1) 1 + (image->width - 1) / 2, or some other similar trick.

Copy link
Copy Markdown
Member

@wantehchang wantehchang Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that by the time we reach here, image->width and image->height have been validated by avifValidateGrid() to be valid AV1 frame width and height. So we know image->width <= 65536 and image->height <= 65536. Therefore we should simply delete this if statement.

Copy link
Copy Markdown
Contributor

@y-guyon y-guyon Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there the same frame dimension limits in AV2?

In general it could be AVIF_ASSERT_OR_RETURN() instead of checks, but leaving the code as is is fine with me too.

return AVIF_RESULT_INVALID_ARGUMENT;
}
uint32_t monoUVWidth = (image->width + 1) >> 1;
uint32_t monoUVHeight = (image->height + 1) >> 1;

// Allocate the U plane if necessary.
if (!avmImageAllocated) {
uint32_t channelSize = avifImageUsesU16(image) ? 2 : 1;
uint32_t monoUVRowBytes = channelSize * monoUVWidth;
if (monoUVHeight > SIZE_MAX / monoUVRowBytes) {
return AVIF_RESULT_INVALID_ARGUMENT;
}
Copy link
Copy Markdown
Member

@wantehchang wantehchang Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also delete this if statement because:

    monoUVWidth <= 32768
    monoUVHeight <= 32768
    channelSize <= 2
    monoUVHeight * monoUVRowBytes <= 32768 * 2 * 32768 < UINT32_MAX <= SIZE_MAX

size_t monoUVSize = (size_t)monoUVHeight * monoUVRowBytes;

monoUVPlane = avifAlloc(monoUVSize);
Expand Down
6 changes: 6 additions & 0 deletions src/codec_svt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,14 @@ static avifResult svtCodecEncodeImage(avifCodec * codec,

#if SVT_AV1_CHECK_VERSION(1, 8, 0)
// Simulate 4:2:0 UV planes. SVT-AV1 does not support 4:0:0 samples.
if (image->width == UINT32_MAX || image->height == UINT32_MAX) {
goto cleanup;
}
const uint32_t uvWidth = (image->width + y_shift) >> y_shift;
const uint32_t uvRowBytes = uvWidth * bytesPerPixel;
if (uvHeight > SIZE_MAX / uvRowBytes) {
goto cleanup;
}
Copy link
Copy Markdown
Member

@wantehchang wantehchang Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y_shift is equal to 1 for 4:2:0. So my analysis in codec_avm.c also applies here. We should delete these two if statements.

const size_t uvSize = (size_t)uvRowBytes * uvHeight;
if (uvSize > UINT32_MAX / 2) {
goto cleanup;
Expand Down
Loading