Coverity doing scans of Wine codebase!

[Colin Wright]

James Hawkins wrote:

> On 4/7/06, Colin Wright <cdwine at tesco.net> wrote:
>>
>> #include <stdio.h>
>> int main(void)
>> {
>>    short int i;
>>    unsigned short int j;
>>    j = 65534;
>>    i = j + 1;
>>    printf("The result is %d\n", i);
>>    return 0;
>> }
>>
> 
> Thanks for the info, but I know C base types limits.  If you'll take a
> look into the code, you'll realize they don't make a difference:
I don't know C very well at all, so that little program was just something
to reassure myself.

I just assumed that srclen was a signed int so its negativity was
independent of the signedness of strlenW.

> 
> I'll go with the hypothetical situation brought up by Tom, that
> strlenW somehow manages to return a value of -3789246 (it would have
> to be a really long string to overflow the limits of int).  We're in
> WideCharToMultiByte and we just set srclen to strlenW(src) + 1 which
> turned out to be -3789245.  Of the possible conversion routines we
> use, we'll start with wine_cpsymbol_wcstombs:
> 
> len = dstlen > srclen ? srclen : dstlen;
> for( i = 0; i < len; i++)
> {...}
> 
> If srclen is negative and dstlen is positive, we use dstlen instead.
Actually, we'd use srclen and end up returning the negative srclen.

len = dstlen > srclen ? srclen : dstlen;
<loop>
if( srclen > len ) return -1;
return len;

Say dstlen == -1 and srclen == -1.
len = ( -1 > -1 ) ? -1 : -1;
len is -1
Skip loop
if( -1 > -1 ) return -1;
return -1;
In this case -1 is returned.

Say dstlen == 1 and srclen = -2
len = ( 1 > -2 ) ? -2 : 1;
len is -2
Skip loop
if ( -2 > -2 ) return -2;
return -2;

/* return -1 on dst buffer overflow, -2 on invalid character */

> I know someone out there will say, "Well what if dstlen is also
> negative?"  In that case someone is going to a lot of trouble to break
> this function, but it still won't work because the loop runs from 0 to
> less than len, and if len is negative, this won't run.  The other
> conversion routines are similar, so we won't go through those.  Now at
> the end of the conversion routines, they return len.  I hear someone
> saying, "Aha!  So we are returning a negative length."

> Bottom of WideCharToMultiByte:
> 
> if (ret < 0)
> {
>     ...
        switch(ret)
        {
        case -1: SetLastError( ERROR_INSUFFICIENT_BUFFER ); break;
        case -2: SetLastError( ERROR_NO_UNICODE_TRANSLATION ); break;
        }
So although it would always return 0 it could set a bogus error.
In the -1 example above we have same-sized buffers so the error is bogus.
Bug.

In the -2 example above we haven't even checked the strings so the error is
bogus.
Bug.

>     ret = 0;
> }
> 
> --
> James Hawkins

-- 
Colin Wright
cdonline at tesco.net