gainmap: prevent integer overflow in plane allocation#3049
gainmap: prevent integer overflow in plane allocation#3049rootvector2 wants to merge 1 commit intoAOMediaCodec:mainfrom
Conversation
Prevent integer overflows in multiplications involving width, height, and rowBytes in src/gainmap.c by performing the multiplications in the size_t type. The size_t type is large enough because pixel buffers for the width, height, and rowBytes have been allocated successfully. "Dexter.k" <164054284+rootvector2@users.noreply.github.com> reported an integer overflow in the allocation of the gainMapF buffers in avifRGBImageComputeGainMap() and suggested a fix in AOMediaCodec#3049.
wantehchang
left a comment
wantehchang
left a comment
There was a problem hiding this comment.
Dexter.k: Thank you very much for the pull request. After I started to review and improve your pull request, I quickly discovered that much more changes were needed. So I audited the entire src/gainmap.c file and wrote #3051. I will treat this pull request as a bug report.
| // --- After this point, the function should exit with 'goto cleanup' to free allocated resources. | ||
| // Overflow protection: 'width * height * sizeof(float)' uses signed int | ||
| // multiplication which is undefined behavior on overflow in C. Compute the | ||
| // allocation size in size_t with explicit overflow checks instead. |
There was a problem hiding this comment.
[All of my review comments are for future reference.]
Omit this kind of comment that explains an overflow check in detail. There are a lot of such overflow checks in libavif. We cannot afford to explain all of them in detail. libavif maintainers are expected to be familiar with them.
| if (gainMapPlaneRowBytes != 0 && baseRgbImage->height > SIZE_MAX / gainMapPlaneRowBytes) { | ||
| res = AVIF_RESULT_INVALID_ARGUMENT; | ||
| goto cleanup; | ||
| } |
There was a problem hiding this comment.
If the width and height are for a buffer that have been allocated successfully, it is safe to multiply width and height in the size_t type. We can take advantage of this fact here. So for the problematic expression width * height * sizeof(float), we only need one overflow check:
const size_t numPixels = (size_t)width * height;
if (numPixels > SIZE_MAX / sizeof(float)) {
res = AVIF_RESULT_INVALID_ARGUMENT;
goto cleanup;
}
const size_t gainMapPlaneSize = numPixels * sizeof(float);
|
Thank you for the detailed review and for auditing the file more thoroughly. I appreciate the improvements in #3051 and am glad the report was useful. |
|
Dexter.k: I noticed you have focused your audit of libavif on calculations of allocation sizes. Since width, height, and rowBytes are all stored in |
Prevent integer overflows in multiplications involving width, height, and rowBytes in src/gainmap.c by performing the multiplications in the size_t type. The size_t type is large enough because pixel buffers for the width, height, and rowBytes have been allocated successfully. "Dexter.k" <164054284+rootvector2@users.noreply.github.com> reported an integer overflow in the allocation of the gainMapF buffers in avifRGBImageComputeGainMap() and suggested a fix in #3049.
2 participants
No description provided.