Skip to content
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

Jump to bottom

Fix overflows in avifRGBImageAllocatePixels() #2354

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Fix overflows in avifRGBImageAllocatePixels() #2354

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 30 additions & 11 deletions src/avif.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,27 +352,33 @@ avifResult avifImageAllocatePlanes(avifImage * image, avifPlanesFlags planes)
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) {
const uint32_t channelSize = avifImageUsesU16(image) ? 2 : 1;
if (image->width > UINT32_MAX / channelSize) {
return AVIF_RESULT_INVALID_ARGUMENT;
}
const size_t fullRowBytes = channelSize * image->width;
if ((fullRowBytes > UINT32_MAX) || (image->height > SIZE_MAX / fullRowBytes)) {
const uint32_t fullRowBytes = channelSize * image->width;
#if UINT32_MAX > PTRDIFF_MAX
// Make sure it is safe to cast image->yuvRowBytes[i] or image->alphaRowBytes to ptrdiff_t.
if (fullRowBytes > PTRDIFF_MAX) {
return AVIF_RESULT_INVALID_ARGUMENT;
}
const size_t fullSize = fullRowBytes * image->height;
#endif
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jzern I added this check yesterday (because I will need to cast rowBytes (what libaom calls stride) to ptrdiff_t when I fix some overflows in src/reformat.c. But I am now wondering if this check is necessary. If we have allocated the plane buffer successfully, it implies that rowBytes is <= SIZE_MAX. So in a reasonable C implentation, this should also imply that rowBytes is <= PTRDIFF_MAX. Do you agree?

cppreference.com describes this unlikely case:

If an array is so large (greater than PTRDIFF_MAX elements, but equal to or less than SIZE_MAX bytes), that the difference between two pointers may not be representable as ptrdiff_t, the result of subtracting two such pointers is undefined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have another idea: we can avoid this unlikely case by changing the check below to use PTRDIFF_MAX:

    if (image->height > PTRDIFF_MAX / fullRowBytes) {
        return AVIF_RESULT_INVALID_ARGUMENT;
    }

This seems to match the following paragraph in cppreference.com:

For char arrays shorter than PTRDIFF_MAX, ptrdiff_t acts as the signed counterpart of size_t: it can store the size of the array of any type and is, on most platforms, synonymous with intptr_t).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using PTRDIFF_MAX sounds reasonable.

if (image->height > SIZE_MAX / fullRowBytes) {
return AVIF_RESULT_INVALID_ARGUMENT;
}
const size_t fullSize = (size_t)fullRowBytes * image->height;

if ((planes & AVIF_PLANES_YUV) && (image->yuvFormat != AVIF_PIXEL_FORMAT_NONE)) {
avifPixelFormatInfo info;
avifGetPixelFormatInfo(image->yuvFormat, &info);

image->imageOwnsYUVPlanes = AVIF_TRUE;
if (!image->yuvPlanes[AVIF_CHAN_Y]) {
image->yuvRowBytes[AVIF_CHAN_Y] = (uint32_t)fullRowBytes;
image->yuvPlanes[AVIF_CHAN_Y] = (uint8_t *)avifAlloc(fullSize);
if (!image->yuvPlanes[AVIF_CHAN_Y]) {
return AVIF_RESULT_OUT_OF_MEMORY;
}
image->yuvRowBytes[AVIF_CHAN_Y] = fullRowBytes;
}

if (!info.monochrome) {
Expand All @@ -381,28 +387,28 @@ avifResult avifImageAllocatePlanes(avifImage * image, avifPlanesFlags planes)
const uint32_t shiftedH = (uint32_t)(((uint64_t)image->height + info.chromaShiftY) >> info.chromaShiftY);

// 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;
const uint32_t uvRowBytes = channelSize * shiftedW;
const size_t uvSize = (size_t)uvRowBytes * shiftedH;

for (int uvPlane = AVIF_CHAN_U; uvPlane <= AVIF_CHAN_V; ++uvPlane) {
if (!image->yuvPlanes[uvPlane]) {
image->yuvRowBytes[uvPlane] = (uint32_t)uvRowBytes;
image->yuvPlanes[uvPlane] = (uint8_t *)avifAlloc(uvSize);
if (!image->yuvPlanes[uvPlane]) {
return AVIF_RESULT_OUT_OF_MEMORY;
}
image->yuvRowBytes[uvPlane] = uvRowBytes;
}
}
}
}
if (planes & AVIF_PLANES_A) {
image->imageOwnsAlphaPlane = AVIF_TRUE;
if (!image->alphaPlane) {
image->alphaRowBytes = (uint32_t)fullRowBytes;
image->alphaPlane = (uint8_t *)avifAlloc(fullSize);
if (!image->alphaPlane) {
return AVIF_RESULT_OUT_OF_MEMORY;
}
image->alphaRowBytes = fullRowBytes;
}
}
return AVIF_RESULT_OK;
Expand Down Expand Up @@ -626,7 +632,20 @@ void avifRGBImageSetDefaults(avifRGBImage * rgb, const avifImage * image)
avifResult avifRGBImageAllocatePixels(avifRGBImage * rgb)
{
avifRGBImageFreePixels(rgb);
const uint32_t rowBytes = rgb->width * avifRGBImagePixelSize(rgb);
const uint32_t pixelSize = avifRGBImagePixelSize(rgb);
if (rgb->width > UINT32_MAX / pixelSize) {
return AVIF_RESULT_INVALID_ARGUMENT;
}
const uint32_t rowBytes = rgb->width * pixelSize;
#if UINT32_MAX > PTRDIFF_MAX
// Make sure it is safe to cast rgb->rowBytes to ptrdiff_t.
if (rowBytes > PTRDIFF_MAX) {
return AVIF_RESULT_INVALID_ARGUMENT;
}
#endif
if (rgb->height > SIZE_MAX / rowBytes) {
return AVIF_RESULT_INVALID_ARGUMENT;
}
rgb->pixels = (uint8_t *)avifAlloc((size_t)rowBytes * rgb->height);
AVIF_CHECKERR(rgb->pixels, AVIF_RESULT_OUT_OF_MEMORY);
rgb->rowBytes = rowBytes;
Expand Down
Loading