[Asterisk-Dev] possible bug in pbx.c::substring() ?

[Luigi Rizzo]

On Mon, Jan 16, 2006 at 01:33:22PM -0600, Tilghman Lesher wrote:
> On Monday 16 January 2006 06:20, Luigi Rizzo wrote:
> > look at the following piece of code in  pbx.c::substring() that
> > deals with incorrect arguments.
> >
> > offset = -N means take the substring starting N positions from the
> > end. In this case (which the test correctly identifies, the max
> > length is -offset); however in the body the max length is incorrectly
> > set to strlen(ret)+offset, while it should be -offset
> >
> >         /* Detect too-long length */
> >         if ((offset < 0 && length > -offset) || (offset >= 0 &&
> > offset+length > strlen(ret))) { if (offset >= 0)
> >                         length = strlen(ret)-offset;
> >                 else
> >                         length = strlen(ret)+offset; /* XXX should be
> > length = -offset */ }
> >
> >
> > right ?
> 
> Try it.  It actually doesn't make any difference.  Both calculations
> result in exactly the same resulting string.

that doesn't mean that the code is correct.
As a matter of fact, the old code may end up writing
outside the buffer, and this makes a very big difference.

Assume the following (inconsistent length in input, but this is
precisely what this code is dealing with; if we did not care we
would just remove all the checks):

	string is "123456789" (so its length is 9)
	offset = -3
	length = 5

the old method computes length = 8 and then executes the following code:

        /* Bounce up to the right offset */
        if (offset >= 0)
                ret += offset;
        else
                ret += strlen(ret)+offset;

        /* Chop off at the requisite length */
        if (length >= 0)
                ret[length] = '\0';

so first ret moves at offset 6 in the output. Then
writes a '\0' at offset 6 + 8 = 14. This is beyond the original length
so may well end up overwriting some other variable in memory.

The only reason you see the same result is that there was already
a '\0' at offset 10.

With the change i am proposing, length is computed as 3, which means
that the terminating '\0' is written at offset 10.
There are other possible fixes of course. as long as it is fixed...

cheers
luigi