Guard JPEG XMP parsing sizes#3249
Conversation
maryla-uc
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for the fix!
Actually allocate a too large xmp_data buffer. Make sure the avifJPEGParseGainMapXMP() call would succeed if the size > INT_MAX check were removed. A follow-up to PR AOMediaCodec#3249.
| GainMapPtr gain_map(avifGainMapCreate()); | ||
| avifBool is_avif_gain_map; | ||
| EXPECT_FALSE(avifJPEGParseGainMapXMP( | ||
| &xmp, static_cast<size_t>(std::numeric_limits<int>::max()) + 1, |
There was a problem hiding this comment.
This test passes an incorrect xmp buffer size to the avifJPEGParseGainMapXMP() function. (The correct xmp buffer size is 1.) We should avoid this.
This test assumes the only thing avifJPEGParseGainMapXMP() does with its xmpSize parameter is to pass it to avifJPEGReadXMLMemory(). We should not assume that.
Another problem with this test is that the xmp buffer does not contain valid data, so if I remove the size > INT_MAX check in avifJPEGReadXMLMemory(), the test still passes (because avifJPEGReadXMLMemory() still fails, for the wrong reason).
I fixed both of these problems in PR #3250.
|
In the summary of this PR, we have:
I have seen this technique in several regression tests recently. The problem with this technique is that the XMP length is not only oversized but also incorrect. Did you write this regression test manually, or by using an AI tool? If you used an AI tool, please instruct the tool to NOT use an incorrect buffer size, i.e., the test should pass the actual allocated buffer size to the function under test. |
|
Thanks for catching this. Yes, the regression test was AI-assisted, and I missed that it passed a size that did not match the actual allocated buffer. That made the test weaker than intended and could let it pass for the wrong reason. I'll make sure future tests pass the actual allocated buffer size and validate the intended failure path. Thanks for fixing it in #3250. |
Summary
Testing