Jacek just sent a patch fixing the following bug:
file = CreateFileW(fileName, GENERIC_READ, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_READONLY,NULL); - if(file) { + if(file != INVALID_HANDLE_VALUE) {
This looks like something that could be turned into a nice Smatch test.
Hint, hint...
Francois Gouget wrote:
Jacek just sent a patch fixing the following bug:
file = CreateFileW(fileName, GENERIC_READ, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_READONLY,NULL);
- if(file) {
- if(file != INVALID_HANDLE_VALUE) {
This looks like something that could be turned into a nice Smatch test.
Hint, hint...
You mean something like http://people.redhat.com/mstefani/wine/smatch/scripts/file_handles.pl ? It found following: dlls/wininet/ftp.c 327 FTP_FtpPutFileW(54) Comparision of the file handle 'hFile' with 0. dlls/wininet/ftp.c 1353 FTP_FtpGetFileW(47) Comparision of the file handle 'hFile' with 0. dlls/rpcrt4/rpcss_np_client.c 92 RPCRT4_RpcssNPConnect(58) Comparision of the file handle 'the_pipe' with 0. dlls/dbghelp/msc.c 2311 pdb_fetch_file_info(21) Comparision of the file handle 'hFile' with 0. dlls/dbghelp/pe_module.c 145 pe_load_dbg_file(52) Comparision of the file handle 'hFile' with 0. programs/rpcss/np_server.c 393 RPCSS_NPConnect(57) Comparision of the file handle 'the_pipe' with 0.
Most are false positives (non NULL check before CloseHandle()). 2-3 are fishy at best.
bye michael
On Wed, 7 Mar 2007, Michael Stefaniuc wrote: [...]
You mean something like http://people.redhat.com/mstefani/wine/smatch/scripts/file_handles.pl ?
Cool, thanks.
[...]
Most are false positives (non NULL check before CloseHandle()).
These are not false positives. Any file handle that is not INVALID_HANDLE_VALUE must be closed with CloseHandle(). So these checks should be against INVALID_HANDLE_VALUE, not NULL. In fact they may possibly be removed altogether.
[...]
dlls/rpcrt4/rpcss_np_client.c 92 RPCRT4_RpcssNPConnect(58) Comparision of the file handle 'the_pipe' with 0. programs/rpcss/np_server.c 393 RPCSS_NPConnect(57) Comparision of the file handle 'the_pipe' with 0.
These two combine with what looks like a very bad file handle leak (especially in rpcrt4).
Sending patches...
Francois Gouget fgouget@free.fr writes:
These are not false positives. Any file handle that is not INVALID_HANDLE_VALUE must be closed with CloseHandle(). So these checks should be against INVALID_HANDLE_VALUE, not NULL. In fact they may possibly be removed altogether.
Note that a valid file handle will never be NULL, so while these checks are wrong in theory, in practice it makes no difference.
Alexandre Julliard wrote:
Francois Gouget fgouget@free.fr writes:
These are not false positives. Any file handle that is not INVALID_HANDLE_VALUE must be closed with CloseHandle(). So these checks should be against INVALID_HANDLE_VALUE, not NULL. In fact they may possibly be removed altogether.
Note that a valid file handle will never be NULL, so while these checks are wrong in theory, in practice it makes no difference.
Well, in one of the found cases (one of the RPCRT) the code was setting the file handle manualy to NULL...
bye michael
On Thu, 8 Mar 2007, Alexandre Julliard wrote: [...]
Note that a valid file handle will never be NULL, so while these checks are wrong in theory, in practice it makes no difference.
Right. But the invalid check means that in some cases we will call CloseHandle(INVALID_HANDLE_VALUE) which the MSDN has this to say about:
If the application is running under a debugger, the function will throw an exception if it receives either a handle value that is not valid or a pseudo-handle value. This can happen if you close a handle twice, or if you call CloseHandle on a handle returned by the FindFirstFile function.
I did not check but I guess Wine does not do that... so far. It's still best avoided.
Francois Gouget fgouget@free.fr writes:
On Thu, 8 Mar 2007, Alexandre Julliard wrote: [...]
Note that a valid file handle will never be NULL, so while these checks are wrong in theory, in practice it makes no difference.
Right. But the invalid check means that in some cases we will call CloseHandle(INVALID_HANDLE_VALUE) which the MSDN has this to say about:
If the application is running under a debugger, the function will throw an exception if it receives either a handle value that is not valid or a pseudo-handle value. This can happen if you close a handle twice, or if you call CloseHandle on a handle returned by the FindFirstFile function.
I did not check but I guess Wine does not do that... so far. It's still best avoided.
Sure, it's best to avoid closing an invalid handle. We don't throw an exception but we do set last error, and this could conceivably break something. But the NULL check is not going to cause us to forget to close a valid handle, which would be a much worse problem.
On Thu, 8 Mar 2007, Alexandre Julliard wrote: [...]
Sure, it's best to avoid closing an invalid handle. We don't throw an exception but we do set last error, and this could conceivably break something. But the NULL check is not going to cause us to forget to close a valid handle, which would be a much worse problem.
Oh, sure.
The exception being RPCRT4_RPCSSOnDemandCall() (and similar code in rpcss) which was so confused it did not call CloseHandle() at all (as far as I could see).
Na , Alexandre Julliard julliard@winehq.org escreveu:
Francois Gouget fgouget@free.fr writes:
These are not false positives. Any file handle that is not INVALID_HANDLE_VALUE must be closed with CloseHandle(). So these checks should be against INVALID_HANDLE_VALUE, not NULL. In fact they may possibly be removed altogether.
Note that a valid file handle will never be NULL, so while these checks are wrong in theory, in practice it makes no difference.
Yes it does. INVALID_HANDLE_VALUE has value of -1. NULL is 0 in a x86 architecture.
Francois Gouget wrote:
On Wed, 7 Mar 2007, Michael Stefaniuc wrote: [...]
You mean something like http://people.redhat.com/mstefani/wine/smatch/scripts/file_handles.pl ?
Cool, thanks.
[...]
Most are false positives (non NULL check before CloseHandle()).
These are not false positives. Any file handle that is not INVALID_HANDLE_VALUE must be closed with CloseHandle(). So these checks should be against INVALID_HANDLE_VALUE, not NULL. In fact they may possibly be removed altogether.
Ok, as there are no false positives i have improved the script a little; documented it on my Smatch page and added it to my daily Smatch run. If you know more functions that return a file_handle i can search for those too. At the moment i'm looking only for the regexp "CreateFile[AW]?".
[...]
dlls/rpcrt4/rpcss_np_client.c 92 RPCRT4_RpcssNPConnect(58) Comparision of the file handle 'the_pipe' with 0. programs/rpcss/np_server.c 393 RPCSS_NPConnect(57) Comparision of the file handle 'the_pipe' with 0.
These two combine with what looks like a very bad file handle leak (especially in rpcrt4).
Sending patches...
Send a patch too for the additional occurence that the improved script has found.
bye michael
Michael Stefaniuc wrote:
Francois Gouget wrote:
On Wed, 7 Mar 2007, Michael Stefaniuc wrote: [...]
Sending patches...
Send a patch too for the additional occurence that the improved script has found.
Duh ... I mean I have sent a patch already for the additional bug found by the improved script.
bye michael
Michael Stefaniuc mstefani@redhat.com writes:
Ok, as there are no false positives i have improved the script a little; documented it on my Smatch page and added it to my daily Smatch run. If you know more functions that return a file_handle i can search for those too. At the moment i'm looking only for the regexp "CreateFile[AW]?".
Probably at least CreateNamedPipe, CreateMailslot and FindFirstFile would be interesting to check.
The opposite check would be nice too, there are probably cases where a call that returns a NULL handle on error is tested against INVALID_HANDLE_VALUE.
Alexandre Julliard wrote:
Michael Stefaniuc mstefani@redhat.com writes:
Ok, as there are no false positives i have improved the script a little; documented it on my Smatch page and added it to my daily Smatch run. If you know more functions that return a file_handle i can search for those too. At the moment i'm looking only for the regexp "CreateFile[AW]?".
Probably at least CreateNamedPipe, CreateMailslot and FindFirstFile would be interesting to check.
Done! Added also FindFirstFileEx(); no new bug was found.
The opposite check would be nice too, there are probably cases where a call that returns a NULL handle on error is tested against INVALID_HANDLE_VALUE.
Can be done but i'll need a list of functions that return HANDLEs. The problem is Smatch looses a lot of type informations and a HANDLE is only an "unsigned long pointer_type". And INVALID_HANDLE_VALUE is just "-1". I have to see if i can automatically generate the list of HANDLE returning functions by other means.
bye michael
On Fri, 9 Mar 2007, Michael Stefaniuc wrote: [...]
Ok, as there are no false positives i have improved the script a little; documented it on my Smatch page and added it to my daily Smatch run.
Thanks a lot for the Smatch script. It has proven pretty useful already.
-
Alexandre Julliard -
Francois Gouget -
João Eiras -
Michael Stefaniuc