Fix overflows in avifRGBImageAllocatePixels() #2354
Conversation
Calculate rowBytes in uint32_t. Only the allocation size needs to be size_t. Also make sure it is safe to cast various rowBytes fields to ptrdiff_t. We need to do this when subtracting rowBytes from a pointer to go back one row. Part of the fix to AOMediaCodec#2271.
| return AVIF_RESULT_INVALID_ARGUMENT; | ||
| } | ||
| const size_t fullSize = fullRowBytes * image->height; | ||
| #endif |
There was a problem hiding this comment.
@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_MAXelements, but equal to or less thanSIZE_MAXbytes), that the difference between two pointers may not be representable asptrdiff_t, the result of subtracting two such pointers is undefined.
There was a problem hiding this comment.
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_tacts as the signed counterpart ofsize_t: it can store the size of the array of any type and is, on most platforms, synonymous withintptr_t).
There was a problem hiding this comment.
Using PTRDIFF_MAX sounds reasonable.
This makese it safe to cast rowBytes to ptrdiff_t. Part of the fix to AOMediaCodec#2354.
This makese it safe to cast rowBytes to ptrdiff_t. Part of the fix to #2354.
Calculate rowBytes in uint32_t. Only the allocation size needs to be size_t.
Also make sure it is safe to cast various rowBytes fields to ptrdiff_t. We need to do this when subtracting rowBytes from a pointer to go back one row.
Part of the fix to #2271.