#505 Infinite loop in lib/xmlparse.c:XML_GetBuffer

[Anonymous]

Hi,
first thanks for maintaining expat.

I found this bug in version 2.0.1 but the code is the same in the current developement version.

When XML_GetBuffer is called and bufferSize is 0 it will be initialised to INIT_BUFFER_SIZE (1024). Which is doubled until it is bigger than needeSize (line 1718). For my example neededSize was
(gdb) p neededSize
$2 = 2128558980

The doubling is optimized to a shift opertaion (gcc 4.7.0). The doubling shifts the true bit in bufferSize out of scope without breaking the loop.

(gdb) p 1024 << 20
$10 = 1073741824
(gdb) p 1024 << 21
$11 = -2147483648
(gdb) p 1024 << 22
$12 = 0

And then goes into an endless loop.

Still searching why the buffer is so huge but i wanted to mention this bug anyway.

Regards.

[Bharat]

Hi this looks great. I started working on it. Please assign it to me.

[Karl Waclawek]

We are grateful for patches to Expat bugs. Patches should be submitted to the Patch tracker. Thank you.

[Bharat]

Hi Karl,

There is a small logical overlooking of extreme cases in lines 1718 - 1722,
in lib / xmlparse.c : XML_GetBuffer

******
if (bufferSize == 0)
bufferSize = INIT_BUFFER_SIZE;
do {
bufferSize *= 2;
} while (bufferSize < neededSize);

******

Here the do - while is going into an infinite loop because of the Shift operation << 1
that the compiler is performing at the line 1721 -> bufferSize *= 2;
Here if bufferSize is 0, this loop is infinite.

Since they are all integers, if the neededSize is anything between [2^30+1, 2^31-1] the bufferSize will eventually shift to become 0 and goes infinite.

To overcome this an easy and simple solution without changing much code could be to add the line,
bufferSize += 1; ( after line number 1721 ).

This will ensure that the condition (bufferSize < neededSize) will be evaluated to false for any value of neededSize, because at some stage bufferSize will be equal to 2^31-1, the highest possible integer value before overflow. Since the allotted buffer size can be dynamic based on our neededSize, it should not change the structure significantly.

final code:
************
if (bufferSize == 0)
bufferSize = INIT_BUFFER_SIZE;
do {
bufferSize *= 2;
bufferSize += 1;
} while (bufferSize < neededSize);

***********
I am new to open source and am sorry I did not know how to submit the patch at the patch tracker. Hoping it helped. If its ok I'd like to look at as many open bugs. I really got to liking expat more after looking through the code :)

[Sebastian Pipping]

This issue was fixed in the mean time:

• In 2.1.1: https://github.com/libexpat/libexpat/commit/ba0f9c3b40c264b8dd392e02a7a060a8fa54f032
• In 2.2.0: https://github.com/libexpat/libexpat/commit/f0bec73b018caa07d3e75ec8dd967f3785d71bde

Closing as fixed.