Grow buffer based on current size

[Snild-Sony]

Until now, the buffer size to grow to has been calculated based on the distance from the current parse position to the end of the buffer. This means that the size of any already-parsed data was not considered, leading to inconsistent buffer growth.

Growing the buffer based on its total size makes its growth consistent.

The commit includes a test that checks that we can reach the max buffer size (usually INT_MAX/2 + 1) regardless of previously parsed content.

[Snild-Sony]

1/2 Test #1: runtests .........................***Failed   14.62 sec
ERROR: check failed: XML_GetBuffer(parser, maxbuf + 1) == NULL
ERROR: check failed: XML_GetBuffer(parser, maxbuf + 1) == NULL
ERROR: check failed: XML_GetBuffer(parser, maxbuf + 1) == NULL

This passes locally for me. I don't see how maxbuf +1 could ever return anything but NULL from GetBuffer(). Either I'm calculating maxbuf wrong, or there's some way for GetBuffer to get around the expected max limit.

And this is correct, isn't it?

const int maxbuf = INT_MAX / 2 + (INT_MAX & 1); // round up without overflow

Looks like it's with the XML_CONTEXT_BYTES=0 config. I'll switch over and see what's going on.

[Snild-Sony]

Breakpoint 3, XML_GetBuffer (parser=0x5555555e55a0, len=1073741825) at xmlparse.c:2079
2079	  if (len > EXPAT_SAFE_PTR_DIFF(parser->m_bufferLim, parser->m_bufferEnd)) {
>p/x len
$19 = 0x40000001
>p parser->m_buffer
$20 = 0x7fff97bff010 "<x a='"
>p parser->m_bufferLim
$21 = 0x7ffff7bff010 ""
>p parser->m_bufferEnd
$22 = 0x7fff97bff016 ""
(gdb) p/x 0x7ffff7bff010 - 0x7fff97bff010
$23 = 0x60000000

The buffer has somehow grown to 0x60000000 bytes. That's not a power of two! But it's suspicously strlen("<x a='") multipled by a power of two.

I'm guessing that we've somehow gotten a buffer of 6 bytes, and gone doubling from there. And that's exactly what happens in this special path in XML_Parse().

So, my patch is incomplete -- we can still get arbitrary (non-power-of-two) buffer sizes through this special path. I'll see what I can do about it.

[Snild-Sony]

My nice little one-liner just grew a bit in complexity -- but now it passes with XML_CONTEXT_BYTES=0 too. :)

[hartwork]

Hi @Snild-Sony, thanks for the new version :+1 Please see the new comments below:

[hartwork]

@Snild-Sony thanks for the replies adjustments! For some reason on MinGW the test on empty string ("") is failing: https://github.com/libexpat/libexpat/actions/runs/6578518007/job/17879007163?pr=771#step:6:490

[Snild-Sony]

I am unable to reproduce that failure locally. I took your https://github.com/libexpat/libexpat/tree/buffer-limit-mingw-experiments and added some extra stuff at https://github.com/SonyMobile/libexpat-contrib/tree/buffer-limit-mingw-experiments -- let's see if the logs show anything interesting.

[Snild-Sony]

They do.

Expat version: expat_2.5.0
[g_chunkSize=0][i=0] BEGIN maxbuf=1073741824
[g_chunkSize=0][test_buffer_can_grow_to_max][i=0/prefix_len=  0] XML_GetBuffer len=1073741824 -> getbuf: len=1073741824
getbuf: neededSize=1073741824
getbuf: need to grow from bufferSize=0
getbuf: new bufferSize=1073741824
getbuf: ERR_NO_MEM because malloc failed, errno=12
failed
[g_chunkSize=0][test_buffer_can_grow_to_max][i=0/prefix_len=  0] XML_GetBuffer len=536870912 -> getbuf: len=536870912
getbuf: neededSize=536870912
getbuf: need to grow from bufferSize=0
getbuf: new bufferSize=536870912
getbuf: reached the end, returning m_bufferEnd=5AFFE020
succeeded
FAIL [chunksize=0]: test_buffer_can_grow_to_max ("" at /home/runner/work/libexpat-contrib/libexpat-contrib/expat/tests/basic_tests.c:2908)
99%: Checks: 348, Failed: 1
ERROR: check failed: XML_GetBuffer(parser, maxbuf - prefix_len) != NULL

Looks like malloc just won't give us the required 1 GiB allocation. This surprises me, since the runners are supposed to have 7GiB of RAM. Since it passes on the "normal" builds, it must be somehow wine- or mingw-specific, but I have the same versions of those packages on one of my machines, and it passes there.

I could make the test check that malloc+free of 1GiB is possible before the actual test, or make it use a custom malloc function that won't fail... but I'd prefer understanding what the actual problem is, if possible. Any guesses, @hartwork?

[hartwork]

@Snild-Sony great to have some first insights and some new ideas 🎉 👍 536870912 is precisely 2^29, interesting! (Oh wait, your loop is halving on integer level every time...) While it should not (have to) affect MinGW on Linux I remember that true Windows malloc is committing to that memory and committing to that much memory in the 32bit world is maybe considered too much and rejected artificially? I'll need to think over this more, maybe even ifdef-ing out the test for MinGW is an option, not anywhere near sure yet.

[Snild-Sony]

committing to that much memory in the 32bit world is maybe considered too much and rejected artifically?

I was thinking along the same lines, but then it should fail with 32-bit wine locally as well -- and it doesn't. There must be some other difference, but I don't know what.

[Snild-Sony]

This new upload adds a workaround for the MinGW failure in CI. On 32-bit, the test now tries to allocate 1GiB, and if it fails, goes down to 512MiB (but no further).

This (still) passes locally, and now hopefully here on GitHub Actions too.

[hartwork]

@Snild-Sony thanks for the adjustments, very nice! 👍

[hartwork]

@Snild-Sony it turns out that clang-format has an issue still:

--- a/expat/tests/basic_tests.c
+++ b/expat/tests/basic_tests.c
@@ -2872,7 +2872,7 @@ START_TEST(test_buffer_can_grow_to_max) {
   if (sizeof(void *) < 8) {
     // Looks like we have a 32-bit system. Can we make a big allocation?
     void *big = malloc(maxbuf);
-    if (!big) {
+    if (! big) {
       // The big allocation failed. Let's be a little lenient.
       maxbuf = maxbuf / 2;
     }

[Snild-Sony]

@Snild-Sony it turns out that clang-format has an issue still:

Järnspikars!

My Ubuntu 22.04 distribution only provides clang up to 15, so #773 has made it hard for me to check locally (clang15 would've caught this one, but has different opinions than clang17 about some other lines).

I've now installed clang 17 directly from the llvm project, so it should be fine going forward.

[hartwork]

@Snild-Sony thanks for the adjustment!

Järnspikars!

Google translate first thinks it's Estonian but when switching to Swedish explicitly it says "Iron Nails!" Probably not too different from "Verdammte Axt!" in German 😃

My Ubuntu 22.04 distribution only provides clang up to 15, so #773 has made it hard for me to check locally (clang15 would've caught this one, but has different opinions than clang17 about some other lines).

I've now installed clang 17 directly from the llvm project, so it should be fine going forward.

Excellent!