Fix integer arithmetic inconsistency in YUV/RGB conversion slow paths#3149
Fix integer arithmetic inconsistency in YUV/RGB conversion slow paths#3149YooLCD wants to merge 3 commits intoAOMediaCodec:mainfrom
Conversation
| avifProperty * alphaAuxProp = avifMetaCreateProperty(meta, "auxC"); | ||
| AVIF_CHECKERR(alphaAuxProp, AVIF_RESULT_OUT_OF_MEMORY); | ||
| strcpy(alphaAuxProp->u.auxC.auxType, AVIF_URN_ALPHA0); | ||
| // Use memcpy instead of strcpy to make the buffer boundary explicit. |
There was a problem hiding this comment.
Nit: Delete this comment.
One problem with this comment is that it describes the change rather than the resulting code.
In general, the change should be described in the commit message of a pull request.
The resulting code, if subtle, can be described in a comment.
| AVIF_CHECKERR(alphaAuxProp, AVIF_RESULT_OUT_OF_MEMORY); | ||
| strcpy(alphaAuxProp->u.auxC.auxType, AVIF_URN_ALPHA0); | ||
| // Use memcpy instead of strcpy to make the buffer boundary explicit. | ||
| // AVIF_URN_ALPHA0 is 43 bytes + null terminator, well within AUXTYPE_SIZE (64). |
There was a problem hiding this comment.
Nit: Delete this comment
libavif recently switched to the ISO C11 standard. This is best valdiated with a static assertion. See pull request #3154.
| const uint8_t * ptrU8 = uPlane ? &uPlane[(uvJ * uRowBytes)] : NULL; | ||
| const uint8_t * ptrV8 = vPlane ? &vPlane[(uvJ * vRowBytes)] : NULL; | ||
| const uint8_t * ptrA8 = aPlane ? &aPlane[j * aRowBytes] : NULL; | ||
| const uint8_t * ptrY8 = &yPlane[(size_t)j * yRowBytes]; |
There was a problem hiding this comment.
Please undo the changes to the avifImageYUVAnyToRGBAnySlow() function. I have previously committed to reviewing pull request #3063 for this function.
There was a problem hiding this comment.
Thank you for the review! I've reverted the changes to avifImageYUVAnyToRGBAnySlow() and removed the comments.
2 participants
In
avifImageYUVAnyToRGBAnySlow()and the YUV420/422 write path ofavifImageRGBToYUV(), chroma row offsets are computed asuvJ * uRowByteswhere both operands areuint32_t. This means the multiplication is done in 32-bit arithmetic before being used as an array index into the UV planes.The fast paths (
avifImageYUV16ToRGB16Color,avifImageYUV8ToRGB8Color, etc.) already guard against this by casting tosize_tfirst:If
yuvRowBytesis provided externally (it is a public field onavifImage), the 32-bit product can wrap on large images. This patch adds(size_t)casts to all 22 affected sites in the slow path and the encode write path to match the fast paths.Also replaces
strcpywithmemcpyinread.cwhen copyingAVIF_URN_ALPHA0intoalphaAuxProp->u.auxC.auxType. No overflow today (AVIF_URN_ALPHA0is 44 bytes,AUXTYPE_SIZEis 64), but every other string copy in the file usesmemcpyoravifROStreamReadString, so this makes it consistent.