Use valid xmp_data buffer in JpegTest.TooLargeXMP

[wantehchang]

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 #3249.

[wantehchang]

cc: @Alb3e3

[wantehchang]

I removed the unnecessary newline and spaces at the end of the xmp string. I then copied this xmp string to the new test.

[maryla-uc]

Should we run the test without allocating in that case? I feel like this more correct test might end up testing less if it gets skipped?
I originally thought the version that didn't allocate was good enough...

[wantehchang]

Indeed, this more correct test hurts test coverage. Note that it is also skipped on 32-bit platforms. See the #if SIZE_MAX > UINT_MAX at line 341.

I understand that not everyone objects to a test that deliberately passes an incorrect buffer size to the function under test. (The reason I object to this practice is that the function under test cannot detect an incorrect buffer size except in trivial cases such as a 0 or negative buffer size.)

In this particular case, the original test has another, perhaps more serious problem: the test still passes if we remove the bug fix. This experiment shows that:

$ git diff
diff --git a/apps/shared/avifjpeg.c b/apps/shared/avifjpeg.c
index 426c5d0a..c1eda541 100644
--- a/apps/shared/avifjpeg.c
+++ b/apps/shared/avifjpeg.c
@@ -582,9 +582,7 @@ static xmlNode * avifJPEGFindXMLNodeByName(const xmlNode * parentNode, const cha
 
 static xmlDoc * avifJPEGReadXMLMemory(const uint8_t * data, size_t size, const char * url)
 {
-    if (size > INT_MAX) {
-        return NULL;
-    }
+    fprintf(stderr, "Passing (int)size=%d to xmlReadMemory()\n", (int)size);
     return xmlReadMemory((const char *)data, (int)size, url, NULL, /*options=*/0);
 }
 
$ tests/avifjpeggainmaptest --gtest_filter=JpegTest.TooLargeXMP ../tests/data/
Note: Google Test filter = JpegTest.TooLargeXMP
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from JpegTest
[ RUN      ] JpegTest.TooLargeXMP
Passing (int)size=-2147483648 to xmlReadMemory()
[       OK ] JpegTest.TooLargeXMP (0 ms)
[----------] 1 test from JpegTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 1 test.

The reason the test still passes is that the avifJPEGReadXMLMemory() function still fails, for the wrong reason. The test is a regression test but it fails to detect the bug.

It's a little tricky to make the test detect the bug. The method I came up with is to make avifJPEGReadXMLMemory() succeed if the bug fix is removed. Unfortunately this method requires a 64-bit size_t type.

[maryla-uc]

Thanks for the extra info. Indeed if it still passed without the bugfix, it was not a very useful test. Your version is better then.