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

open
nobody
None
5
2012-07-09
2012-07-09
Anonymous
No

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.

Discussion

  • Bharat
    Bharat
    2012-07-24

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

     
  • Karl Waclawek
    Karl Waclawek
    2012-07-24

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

     
  • Bharat
    Bharat
    2012-07-26

    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 :)