FreeTDS Development Group

["James K. Lowden" <jklowden AT schemamania.org>]

On 15 Mar 2003 12:23:18 +0100, Frediano Ziglio <freddyz77 AT tin.it> wrote:
> I found a new, strange assert in dblib.c (dbconvert)
> 
>               case -1:        /* rtrim and null terminate */
>                       for (i = len - 1; i >= 0 && dres.c[i] == ' '; --i) {
>                               len = i;
>                       }
>                       assert(dres.c[len] == ' ');     /* if last position 
> is not a
>                       blank, no
> room to null terminate */
>                       memcpy(dest, dres.c, len);
>                       dest[len] = '\0';
>                       ret = len;
>                       break;
> 
> Why should dres.c contain space for terminator if copied and terminated
> in copied buffer?
> Also dres.c is not terminated, so this assert cause a core in some
> condition (it's a read buffer overflow)...

The assert is wrong, and so is the code, still.  

We are going to write dest[len], but "len" is the count of bytes, returned
from tds_convert.  "len" can be decreased by the for() loop, but that loop
won't execute if dres.c[len-1] is not a blank.  There's no assurance that
"len" fits in dest.  

The assert should have been:

        assert(len < destlen);

What do you say?

--jkl