Coverity doing scans of Wine codebase!
See http://scan.coverity.com/ They say they've found 830 potential bugs - that's quite a few. I haven't seen 'em yet, still waiting for my registration to come through. - Dan -- Wine for Windows ISVs: http://kegel.com/wine/isv
* Dan Kegel <dank(a)kegel.com> [05/04/06, 22:49:13]:
See http://scan.coverity.com/ They say they've found 830 potential bugs - that's quite a few. I haven't seen 'em yet, still waiting for my registration to come through.
From the coverity post on the samba-technical list I gather that they'd prefer if only core developer signed up. I'm not sure if they changed their stance. If they didn't, do you think it'd be possible to get an overview? If every wine dev can sign up, we can of course sign up ourselves.
Cheers, Kai -- Kai Blin, (blin at gmx dot net) Tallulah Bankhead barged down the Nile last night as Cleopatra and sank. -- John Mason Brown, drama critic
Yes, this is awesome news, so far every one I have checked is a real bug. Coverity seem to be getting great PR from this and rightly so, the "Stanford Checker", whatever it is, seems very effective. Anyway most of these look like easy bugs to fix. I see several of us already have access - so, patches away!
On Thu, 06 Apr 2006 20:39:00 +0100, Mike Hearn wrote:
Yes, this is awesome news, so far every one I have checked is a real bug.
OK, that was a bit over-enthusiastic. A few of these are more tricky. EG: * One was wrong, it didn't track the fact that the given variable was initialized by a subroutine * Another (missing NULL ptr check in LoadTypeLibEx) is right, but, I don't think we want to add lots of missing NULL arg checks in the public API implementations. An application will never pass NULL to this function directly as otherwise it'd not work at all, so, a crash with a NULL arg here probably is revealing some other bug. I'd rather it crashed cleanly in a debuggable way than silently return error code and continue, in other words ... * It has identified a codepath through the server window station code where a struct desktop could be dereffed without being initialized. But I am not sure if this codepath is logically possible. Somebody more familiar with that code (eg Alexandre) would have to check if it could actually ever be taken or not. * Some of these are bugs that aren't really a high priority, eg leaks in winegcc (which doesn't live very long anyway) Still. A real treasure trove of data here. Thanks Ben!
Hi, On Thu, Apr 06, 2006 at 08:54:17PM +0100, Mike Hearn wrote:
* Another (missing NULL ptr check in LoadTypeLibEx) is right, but, I don't think we want to add lots of missing NULL arg checks in the public API implementations. An application will never pass NULL to this function directly as otherwise it'd not work at all, so, a crash with a NULL arg here probably is revealing some other bug.
I'd rather it crashed cleanly in a debuggable way than silently return error code and continue, in other words ... Yes, as discussed before, this kind of "bugs" most likely should *not* be fixed. Examples of real bugs here would be a missing NULL pointer in *Wine internal* code that really should have had a NULL pointer check since it's dealing with exclusively internal data (i.e. data that has a rather closed life cycle within a certain wine mechanism, without exposure to the public Win32 side of things).
Andreas -- No programming skills!? Why not help translate many Linux applications! https://launchpad.ubuntu.com/rosetta (or alternatively buy nicely packaged Linux distros/OSS software to help support Linux developers creating shiny new things for you?)
On Thu, 06 Apr 2006 22:04:14 +0200, Andreas Mohr wrote:
Examples of real bugs here would be a missing NULL pointer in *Wine internal* code that really should have had a NULL pointer check since it's dealing with exclusively internal data (i.e. data that has a rather closed life cycle within a certain wine mechanism, without exposure to the public Win32 side of things).
Yes, maybe, even then if a NULL ptr is not logically possible at that point then it's not really a bug to just go ahead and use it, is it? The checker seems to do a reasonably good job of detecting such things, but even so, it doesn't seem able to reliably trace code like: NTSTATUS set_the_foo(char **foo) { if (whatever) return STATUS_WHATEVER; *foo = "biz"; return STATUS_SUCCESS; } char *foo; if (set_the_foo(&foo) == STATUS_SUCCESS) printf("%c\n", foo[0]); thanks -mike
Mike Hearn wrote:
On Thu, 06 Apr 2006 20:39:00 +0100, Mike Hearn wrote:
Yes, this is awesome news, so far every one I have checked is a real bug.
OK, that was a bit over-enthusiastic. A few of these are more tricky. EG:
* One was wrong, it didn't track the fact that the given variable was initialized by a subroutine
* Another (missing NULL ptr check in LoadTypeLibEx) is right, but, I don't think we want to add lots of missing NULL arg checks in the public API implementations. An application will never pass NULL to this function directly as otherwise it'd not work at all, so, a crash with a NULL arg here probably is revealing some other bug.
Both of these were detected by internal inconsistencies. For example: void foo(char *str) { *str = '\0'; ... if (str) strcpy(str, "bar"); } It is NOT assuming that every function could receive a NULL, although it looks like it does analysis within the same file to see if the function could be called with NULL.
* It has identified a codepath through the server window station code where a struct desktop could be dereffed without being initialized. But I am not sure if this codepath is logically possible. Somebody more familiar with that code (eg Alexandre) would have to check if it could actually ever be taken or not.
I have a patch for this and yes it is logically possible. (Don't argue with a computer over logic, it is far better than any human at it!) -- Rob Shearman
Some of the "bugs" it picks up are cases of defensive programming, such as checking for a NULL pointer even though the NULL pointer is impossible, given the functioning of routines that are being called. What should the policy be on such bugs - should we remove the unnecessary check or keep the extra-defensive code? -- Troy Rollo - wine(a)troy.rollo.name
On 4/6/06, Troy Rollo <wine(a)troy.rollo.name> wrote:
Some of the "bugs" it picks up are cases of defensive programming, such as checking for a NULL pointer even though the NULL pointer is impossible, given the functioning of routines that are being called. What should the policy be on such bugs - should we remove the unnecessary check or keep the extra-defensive code?
In most internal functions we should not have the check so we crash if we ever send in a NULL ptr (in cases where we should never send in NULL.) Mosts of the win32 API should have checks, but the test suite verifies these for sure. -- James Hawkins
When changing the status of an item in the Coverity database it would be helpful if people making the change put in a comment to indicate the basis on which the decision was made - much like an SCM commit log entry it will help anybody else who ends up looking at the item later, whether to double-check the decision or to fix the bug. -- Troy Rollo - wine(a)troy.rollo.name
On 4/6/06, Mike Hearn <mike(a)plan99.net> wrote:
OK, that was a bit over-enthusiastic. A few of these are more tricky. EG:
Of the possible bugs I've seen so far, most of them are valid and worth fixing, but the checker stumbles over WideCharToMultiByte. The checker reports two errors for (most) calls to WideCharToMultibyte: * Passing a negative value for the length of the source string. The checker doesn't pick up on this line: if (srclen < 0) srclen = strlenW(src) + 1; so we never access the string with a negative index. * Negative value can be returned and we don't check for it. The return type of WideCharToMultiByte is int, but the function is only supposed to return string lengths or 0 on error, and AFAIK no negative value is ever returned. You would think the checker would pick up on that fact. The problem is that we call WideCharToMultiByte quite a few times throughout the wine codebase, so we have a lot of false positives with that one. -- James Hawkins
James Hawkins wrote:
On 4/6/06, Mike Hearn <mike(a)plan99.net> wrote:
OK, that was a bit over-enthusiastic. A few of these are more tricky. EG:
Of the possible bugs I've seen so far, most of them are valid and worth fixing, but the checker stumbles over WideCharToMultiByte. The checker reports two errors for (most) calls to WideCharToMultibyte:
* Passing a negative value for the length of the source string.
The checker doesn't pick up on this line:
if (srclen < 0) srclen = strlenW(src) + 1;
so we never access the string with a negative index.
Umm, all that does is increment it by 1... What if _somehow_ (dont ask me how, just venturing a guess) a bogus number is passed by strlenW(src) like -3789246? Then you end up with srclen == -3789245...
* Negative value can be returned and we don't check for it.
The return type of WideCharToMultiByte is int, but the function is only supposed to return string lengths or 0 on error, and AFAIK no negative value is ever returned. You would think the checker would pick up on that fact.
The problem is that we call WideCharToMultiByte quite a few times throughout the wine codebase, so we have a lot of false positives with that one.
I could be wrong, but wouldnt it be (theoretically speaking) possible for a program to force a negative number out of it (even though it isnt supposed to be able to), since it IS an int, regardless of the return value type? Tom
On 4/7/06, Tom Spear (Dustin Booker, Dustin Navea) <speeddymon(a)gmail.com> wrote:
if (srclen < 0) srclen = strlenW(src) + 1;
so we never access the string with a negative index.
Umm, all that does is increment it by 1... What if _somehow_ (dont ask me how, just venturing a guess) a bogus number is passed by strlenW(src) like -3789246? Then you end up with srclen == -3789245...
strlen returns a value of type size_t, which is an unsigned value, so this is always going to be positive.
* Negative value can be returned and we don't check for it.
I could be wrong, but wouldnt it be (theoretically speaking) possible for a program to force a negative number out of it (even though it isnt supposed to be able to), since it IS an int, regardless of the return value type?
If I write a function foo(x,y) that returns a signed int, but I only ever return a value >= 0, then no matter what input the user gives for x and y, a negative value can never be returned. The problem is that MS decided to make the return type of WideCharToMultiByte int instead of unsigned int, which we can't change. Let's say hypothetically that a negative value can be returned in our implementation, then it's still a bug in WideCharToMultiByte and these Coverity bugs are still false positives. -- James Hawkins
James Hawkins wrote:
On 4/7/06, Tom Spear (Dustin Booker, Dustin Navea) <speeddymon(a)gmail.com> wrote:
if (srclen < 0) srclen = strlenW(src) + 1;
so we never access the string with a negative index.
Umm, all that does is increment it by 1... What if _somehow_ (dont ask me how, just venturing a guess) a bogus number is passed by strlenW(src) like -3789246? Then you end up with srclen == -3789245...
strlen returns a value of type size_t, which is an unsigned value, so this is always going to be positive.
But strlenW returns an int. I think this is the thing that Coverity is picking up on. -- Rob Shearman
On 4/7/06, Robert Shearman <rob(a)codeweavers.com> wrote:
strlen returns a value of type size_t, which is an unsigned value, so this is always going to be positive.
But strlenW returns an int. I think this is the thing that Coverity is picking up on.
strlenW from include/wine/unicode.h returns unsigned int :) -- James Hawkins
James Hawkins wrote:
On 4/7/06, Robert Shearman <rob(a)codeweavers.com> wrote:
strlen returns a value of type size_t, which is an unsigned value, so this is always going to be positive.
But strlenW returns an int. I think this is the thing that Coverity is picking up on.
strlenW from include/wine/unicode.h returns unsigned int :)
-- James Hawkins
#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; } Colin
On 4/7/06, Colin Wright <cdwine(a)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'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. 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) { ... ret = 0; } -- James Hawkins
James Hawkins wrote:
On 4/7/06, Colin Wright <cdwine(a)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(a)tesco.net
On 4/8/06, Colin Wright <cdonline(a)tesco.net> wrote:
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.
Like I said before, this isn't about WideCharToMultiByte being 'correct'; it's about the fact that Coverity's analysis of this function is wrong, leading to a false positive. The only person that's going to send in a string long enough to overflow the size of an int isn't going to care about the last error, only that the attack didn't work, because we don't return a negative length and we don't access the string with a negative index. Besides, anyone can overflow strlen with a long enough string in (I imagine) most libraries, so there's not much we can do about that. The expected behavior of this function goes out the window if a user uses a string that long, and we can't consider these bugs in that case. -- James Hawkins
James Hawkins wrote:
On 4/8/06, Colin Wright <cdonline(a)tesco.net> wrote:
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.
Like I said before, this isn't about WideCharToMultiByte being 'correct'; it's about the fact that Coverity's analysis of this function is wrong, leading to a false positive. The only person that's going to send in a string long enough to overflow the size of an int isn't going to care about the last error, only that the attack didn't work, because we don't return a negative length and we don't access the string with a negative index. Besides, anyone can overflow strlen with a long enough string in (I imagine) most libraries, so there's not much we can do about that. The expected behavior of this function goes out the window if a user uses a string that long, and we can't consider these bugs in that case.
-- James Hawkins
OK. I haven't read the details of the Coverity bug entry, so I didn't realise that it was saying that the impact of srclen < 0 was the use of a negative index. There can be copying of the massive string by wcstombs_sbcs_slow, called by wine_cp_wcstombs, called by WideCharToMultiByte, but that increments the destination pointer so there wouldn't be access using a negative index. Colin cdwine(a)tesco.net
On 4/7/06, James Hawkins <truiken(a)gmail.com> wrote:
On 4/7/06, Tom Spear (Dustin Booker, Dustin Navea) <speeddymon(a)gmail.com> wrote:
if (srclen < 0) srclen = strlenW(src) + 1;
so we never access the string with a negative index.
Umm, all that does is increment it by 1... What if _somehow_ (dont ask me how, just venturing a guess) a bogus number is passed by strlenW(src) like -3789246? Then you end up with srclen == -3789245...
strlen returns a value of type size_t, which is an unsigned value, so this is always going to be positive.
* Negative value can be returned and we don't check for it.
I could be wrong, but wouldnt it be (theoretically speaking) possible for a program to force a negative number out of it (even though it isnt supposed to be able to), since it IS an int, regardless of the return value type?
If I write a function foo(x,y) that returns a signed int, but I only ever return a value >= 0, then no matter what input the user gives for x and y, a negative value can never be returned. The problem is that MS decided to make the return type of WideCharToMultiByte int instead of unsigned int, which we can't change. Let's say hypothetically that a negative value can be returned in our implementation, then it's still a bug in WideCharToMultiByte and these Coverity bugs are still false positives.
That is true, but we also need to make sure that since we are going for
conformity (including conforming to MS's bugs) we don't fix a bug in our code that is reported by Coverity, but that is also a bug in MS code.. So if a game has to work around a bug in MS code, then our code should still have that bug (so the game will work correctly), even if Coverity picks it up.... Tom
Tom Spear wrote:
That is true, but we also need to make sure that since we are going for conformity (including conforming to MS's bugs) we don't fix a bug in our code that is reported by Coverity, but that is also a bug in MS code.. So if a game has to work around a bug in MS code, then our code should still have that bug (so the game will work correctly), even if Coverity picks it up....
Such MS compatibility bugs should be better documented in the code and checked by a Wine test else the danger that it will be removed is too big. bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 Sr. Network Engineer Fax.: +49-711-96437-111 Red Hat GmbH Email: mstefani(a)redhat.com Hauptstaetterstr. 58 http://www.redhat.de/ D-70178 Stuttgart
On 4/7/06, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
Tom Spear wrote:
That is true, but we also need to make sure that since we are going for conformity (including conforming to MS's bugs) we don't fix a bug in our code that is reported by Coverity, but that is also a bug in MS code.. So if a game has to work around a bug in MS code, then our code should still have that bug (so the game will work correctly), even if Coverity picks it up....
Such MS compatibility bugs should be better documented in the code and checked by a Wine test else the danger that it will be removed is too big.
I couldn't agree more. Matter of fact I recall that this is the reason that the KLEZ virus from a few years ago was able to infect a linux system running outlook express on wine, as well as why certain games crash (they work around a bug that doesn't exist in our code), so at the time, AJ said that if windows has a bug, so should wine. I was puzzled by it at the time, but since then I figured it out (previous note in () ). :-) Tom P.S. AJ, how should we go about determining what bugs that Coverity picks up are bugs that exist in windows, so that they can be marked in the actual code (in order to prevent regressions)?
Tom Spear wrote:
On 4/7/06, *Michael Stefaniuc* <mstefani(a)redhat.com
I couldn't agree more. Matter of fact I recall that this is the reason that the KLEZ virus from a few years ago was able to infect a linux system running outlook express on wine, as well as why certain games crash (they work around a bug that doesn't exist in our code), so at the time, AJ said that if windows has a bug, so should wine. I was puzzled by it at the time, but since then I figured it out (previous note in () ). :-)
Tom
P.S. AJ, how should we go about determining what bugs that Coverity picks up are bugs that exist in windows, so that they can be marked in the actual code (in order to prevent regressions)?
That's what regression tests are for, right? just me thinking aloud ;-) regards, Joris
Mike Hearn wrote:
* Another (missing NULL ptr check in LoadTypeLibEx) is right, but, I don't think we want to add lots of missing NULL arg checks in the public API implementations. An application will never pass NULL to this function directly as otherwise it'd not work at all, so, a crash with a NULL arg here probably is revealing some other bug.
I'd rather it crashed cleanly in a debuggable way than silently return error code and continue, in other words ...
Is there a way to tell the code checker to skip the NULL check? Maybe there are flags like '__user' in the kernel source -- '__notnull'. tom
Tomas Carnecky wrote:
* Another (missing NULL ptr check in LoadTypeLibEx) is right, but, I don't think we want to add lots of missing NULL arg checks in the public API implementations. An application will never pass NULL to this function directly as otherwise it'd not work at all, so, a crash with a NULL arg here probably is revealing some other bug.
I'd rather it crashed cleanly in a debuggable way than silently return error code and continue, in other words ...
Is there a way to tell the code checker to skip the NULL check? Maybe there are flags like '__user' in the kernel source -- '__notnull'.
It's complaining because there is already a NULL check further down in the code, but we use the pointer without checking for NULL first. In this case, dlls/oleaut32/typelib.c#343 calls TLB_ReadTypeLib with no null check, but there is a NULL check at #350. So either: 1) We do need a NULL check, and we should check the pointer before using it OR 2) We don't need a NULL check, and we should get rid of the unnecessary check. If the function never checks that parameter for NULL, the checker won't complain about it. Please be careful before writing off a warning from the checker as "not a bug". Mike
Mike McCormack wrote:
Tomas Carnecky wrote:
* Another (missing NULL ptr check in LoadTypeLibEx) is right, but, I don't think we want to add lots of missing NULL arg checks in the public API implementations. An application will never pass NULL to this function directly as otherwise it'd not work at all, so, a crash with a NULL arg here probably is revealing some other bug.
I'd rather it crashed cleanly in a debuggable way than silently return error code and continue, in other words ...
Is there a way to tell the code checker to skip the NULL check? Maybe there are flags like '__user' in the kernel source -- '__notnull'.
It's complaining because there is already a NULL check further down in the code, but we use the pointer without checking for NULL first.
If the function never checks that parameter for NULL, the checker won't complain about it.
Ah.. I didn't know that. tom
If the function never checks that parameter for NULL, the checker won't complain about it.
OK, I didn't realise that, thanks.
Please be careful before writing off a warning from the checker as "not a bug".
I'll go back over the ones I marked as such and double check them for this.
participants (15)
-
Andreas Mohr -
Colin Wright -
Colin Wright -
Dan Kegel -
James Hawkins -
Joris Huizer -
Kai Blin -
Michael Stefaniuc -
Mike Hearn -
Mike McCormack -
Robert Shearman -
Tom Spear -
Tom Spear (Dustin Booker, Dustin Navea)
-
Tomas Carnecky -
Troy Rollo