Here is the run for Friday Sept. 11 with the tools and the tests directory results removed.
[/home/cahrendt/wine-git/dlls/ntdll/server.c:802]: (error) Resource leak: fd [/home/cahrendt/wine-git/dlls/ntdll/server.c:882]: (error) Resource leak: fd_cwd [/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
Chris
2009/9/12 chris ahrendt celticht32@yahoo.com:
Here is the run for Friday Sept. 11 with the tools and the tests directory results removed.
[/home/cahrendt/wine-git/dlls/ntdll/server.c:802]: (error) Resource leak: fd [/home/cahrendt/wine-git/dlls/ntdll/server.c:882]: (error) Resource leak: fd_cwd [/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
Chris
Hi Chris,
All false positives, you can filter them.
On Sat, Sep 12, 2009 at 11:19 AM, Nicolas Le Cam niko.lecam@gmail.com wrote:
2009/9/12 chris ahrendt celticht32@yahoo.com:
Here is the run for Friday Sept. 11 with the tools and the tests directory results removed.
[/home/cahrendt/wine-git/dlls/ntdll/server.c:802]: (error) Resource leak: fd [/home/cahrendt/wine-git/dlls/ntdll/server.c:882]: (error) Resource leak: fd_cwd [/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
Chris
Hi Chris,
All false positives, you can filter them.
-- Nicolas Le Cam
They're not false positives. The first one may be (tell cppcheck guys that if we're calling exit on all paths, we shouldn't bother to free resources), but the next few aren't.
fd_cwd is leaked if server connection fails, the bug in wineps is very real, we deref before we check for null on the next line.
Mike.
On Sat, Sep 12, 2009 at 11:24 AM, Mike Kaplinskiy mike.kaplinskiy@gmail.com wrote:
On Sat, Sep 12, 2009 at 11:19 AM, Nicolas Le Cam niko.lecam@gmail.com wrote:
2009/9/12 chris ahrendt celticht32@yahoo.com:
Here is the run for Friday Sept. 11 with the tools and the tests directory results removed.
[/home/cahrendt/wine-git/dlls/ntdll/server.c:802]: (error) Resource leak: fd [/home/cahrendt/wine-git/dlls/ntdll/server.c:882]: (error) Resource leak: fd_cwd [/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
Chris
Hi Chris,
All false positives, you can filter them.
-- Nicolas Le Cam
They're not false positives. The first one may be (tell cppcheck guys that if we're calling exit on all paths, we shouldn't bother to free resources), but the next few aren't.
fd_cwd is leaked if server connection fails, the bug in wineps is very real, we deref before we check for null on the next line.
Mike.
Sorry, sent a little too quickly, the fd_cwd case being leaked is also the same exit false positive. You can ignore these two for next time.
Mike.
2009/9/12 Mike Kaplinskiy mike.kaplinskiy@gmail.com:
On Sat, Sep 12, 2009 at 11:24 AM, Mike Kaplinskiy mike.kaplinskiy@gmail.com wrote:
On Sat, Sep 12, 2009 at 11:19 AM, Nicolas Le Cam niko.lecam@gmail.com wrote:
2009/9/12 chris ahrendt celticht32@yahoo.com:
Here is the run for Friday Sept. 11 with the tools and the tests directory results removed.
[/home/cahrendt/wine-git/dlls/ntdll/server.c:802]: (error) Resource leak: fd [/home/cahrendt/wine-git/dlls/ntdll/server.c:882]: (error) Resource leak: fd_cwd [/home/cahrendt/wine-git/dlls/wineps.drv/init.c:270]: (error) Possible null pointer dereference: dmW - otherwise it is redundant to check if dmW is null at line 272
Chris
Hi Chris,
All false positives, you can filter them.
-- Nicolas Le Cam
They're not false positives. The first one may be (tell cppcheck guys that if we're calling exit on all paths, we shouldn't bother to free resources), but the next few aren't.
fd_cwd is leaked if server connection fails, the bug in wineps is very real, we deref before we check for null on the next line.
Mike.
Sorry, sent a little too quickly, the fd_cwd case being leaked is also the same exit false positive. You can ignore these two for next time.
Mike.
Last one is also a false positive, it's just two pointers being subtracted to retrieve an offset.
2009/9/13 Nicolas Le Cam niko.lecam@gmail.com:
Last one is also a false positive, it's just two pointers being subtracted to retrieve an offset.
That's not the reason why it's a false positive. Without context that line does look like a NULL-dereference (dmW is dereferenced to get the first pointer before any NULL checking). However it's in a static function that is called in two places, both of which already check the pointer being dereferenced.
2009/9/13 Ben Klein shacklein@gmail.com:
2009/9/13 Nicolas Le Cam niko.lecam@gmail.com:
Last one is also a false positive, it's just two pointers being subtracted to retrieve an offset.
That's not the reason why it's a false positive. Without context that line does look like a NULL-dereference (dmW is dereferenced to get the first pointer before any NULL checking). However it's in a static function that is called in two places, both of which already check the pointer being dereferenced.
No, that's not how it works. The expression "dmW->dmFormName" by itself doesn't dereference anything, it's just an address. Only once you read or write something from/to that address does dmW being NULL (or otherwise invalid) become a problem. Here we just subtract "dmW" from that address to get the field offset of "dmFormName" in the DEVMODEW structure. Note that the value of "off_formname" doesn't depend on the value of "dmW", just on its type. The compiler will probably recognize that and optimize it away. Arguably the code in question should just use the FIELD_OFFSET macro though.
On Sat, Sep 12, 2009 at 7:08 PM, Henri Verbeet hverbeet@gmail.com wrote:
2009/9/13 Ben Klein shacklein@gmail.com:
2009/9/13 Nicolas Le Cam niko.lecam@gmail.com:
Last one is also a false positive, it's just two pointers being subtracted to retrieve an offset.
That's not the reason why it's a false positive. Without context that line does look like a NULL-dereference (dmW is dereferenced to get the first pointer before any NULL checking). However it's in a static function that is called in two places, both of which already check the pointer being dereferenced.
No, that's not how it works. The expression "dmW->dmFormName" by itself doesn't dereference anything, it's just an address. Only once you read or write something from/to that address does dmW being NULL (or otherwise invalid) become a problem. Here we just subtract "dmW" from that address to get the field offset of "dmFormName" in the DEVMODEW structure. Note that the value of "off_formname" doesn't depend on the value of "dmW", just on its type. The compiler will probably recognize that and optimize it away. Arguably the code in question should just use the FIELD_OFFSET macro though.
Actually it does dereference something, if you think of dmFormName being an int (not a pointer), then you would be subtracting an address from a random value. If this being an offset is indeed the intent of this code, you need a & in front of dmW->dmFormName so the expression would read
ptrdiff_t off_formname = (const char *)(&dmW->dmFormName) - (const char *)dmW;
But as Ben noted, the cleaner way would be to use FIELD_OFFSET, which does exactly the above.
As for the context note, it is perfectly valid code (segfault-less, that is) as it stands, but we should either remove the null check on the next line or assign the value later.
Mike.
2009/9/13 Mike Kaplinskiy mike.kaplinskiy@gmail.com:
But as Ben noted, the cleaner way would be to use FIELD_OFFSET, which does exactly the above.
As much as I'd love to take the credit for suggesting FIELD_OFFSET, it was Henri :)
2009/9/13 Mike Kaplinskiy mike.kaplinskiy@gmail.com:
Actually it does dereference something, if you think of dmFormName being an int (not a pointer), then you would be subtracting an address from a random value.
If it were an int, sure, but "dmFormName" is a WCHAR array.
2009/9/13 Henri Verbeet hverbeet@gmail.com:
2009/9/13 Mike Kaplinskiy mike.kaplinskiy@gmail.com:
Actually it does dereference something, if you think of dmFormName being an int (not a pointer), then you would be subtracting an address from a random value.
If it were an int, sure, but "dmFormName" is a WCHAR array.
My point was that dmW->foo dereferences dmW to get to foo, being the equivalent of (*dmW).foo. However at present there is no possibility of NULL-dereferencing here as the dmW pointer is validated in the parent functions.
Mike Kaplinskiy mike.kaplinskiy@gmail.com writes:
As for the context note, it is perfectly valid code (segfault-less, that is) as it stands, but we should either remove the null check on the next line or assign the value later.
No, the code is fine as it is. The bug is in cppcheck.
-
Alexandre Julliard -
Ben Klein -
chris ahrendt -
Henri Verbeet -
Mike Kaplinskiy -
Nicolas Le Cam