Re: Shell32: Check null handles before freeing
On 25.11.2015 10:02, Olivier F. R. Dierick wrote:
Fix one of the error leak source that prevents the installer to succeed (bug 36838).
The original code did not check if the handles were null before freeing them. For some reason they happen to be null and last error is set to ERROR_INVALID_HANDLE. The game installer chokes at some point when the last error is anything but zero and fails to complete the installation. Checking if the handles are non-null before freeing them avoids this. According to MSDN, it is not an error to have null values for those handles.
Signed-off-by: Olivier F. R. Dierick <o.dierick(a)piezo-forte.be> --- dlls/shell32/changenotify.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
We most likely have many more places that do same thing, it will need tests for SHFree() and CoTaskMemFree(). If it turns out error is not changed on NULLs, it's easier to return early from SHFree or CoTaskMemFree than fix all places where those are used.
Le mercredi 25 novembre 2015 à 11:11 +0300, Nikolay Sivov a écrit :
On 25.11.2015 10:02, Olivier F. R. Dierick wrote:
Fix one of the error leak source that prevents the installer to succeed (bug 36838).
The original code did not check if the handles were null before freeing them. For some reason they happen to be null and last error is set to ERROR_INVALID_HANDLE. The game installer chokes at some point when the last error is anything but zero and fails to complete the installation. Checking if the handles are non-null before freeing them avoids this. According to MSDN, it is not an error to have null values for those handles.
Signed-off-by: Olivier F. R. Dierick <o.dierick(a)piezo-forte.be> --- dlls/shell32/changenotify.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
We most likely have many more places that do same thing, it will need tests for SHFree() and CoTaskMemFree(). If it turns out error is not changed on NULLs, it's easier to return early from SHFree or CoTaskMemFree than fix all places where those are used.
Hello, I sent a test for CoTaskMemFree() to the Wine test bot. The test passed with 0 failures. Can someone confirm that the test is valid? Here is the link to the test job: https://testbot.winehq.org/JobDetails.pl?Key=18703 Also, I put the test in a new file (dlls/ole32/tests/ifs.c). This file contains only one test. Is it worth submission? -- Olivier F. R. Dierick o.dierick(a)piezo-forte.be
On 01.12.2015 22:02, Olivier F. R. Dierick wrote:
Le mercredi 25 novembre 2015 à 11:11 +0300, Nikolay Sivov a écrit :
On 25.11.2015 10:02, Olivier F. R. Dierick wrote:
Fix one of the error leak source that prevents the installer to succeed (bug 36838).
The original code did not check if the handles were null before freeing them. For some reason they happen to be null and last error is set to ERROR_INVALID_HANDLE. The game installer chokes at some point when the last error is anything but zero and fails to complete the installation. Checking if the handles are non-null before freeing them avoids this. According to MSDN, it is not an error to have null values for those handles.
Signed-off-by: Olivier F. R. Dierick <o.dierick(a)piezo-forte.be> --- dlls/shell32/changenotify.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
We most likely have many more places that do same thing, it will need tests for SHFree() and CoTaskMemFree(). If it turns out error is not changed on NULLs, it's easier to return early from SHFree or CoTaskMemFree than fix all places where those are used.
Hello,
I sent a test for CoTaskMemFree() to the Wine test bot. The test passed with 0 failures. Can someone confirm that the test is valid? Here is the link to the test job: https://testbot.winehq.org/JobDetails.pl?Key=18703
Test is valid, but todo_wine is not - it doesn't fail for me in Wine, without todo.
Also, I put the test in a new file (dlls/ole32/tests/ifs.c). This file contains only one test. Is it worth submission?
In general I think testing last errors from OLE and COM methods/functions is not very useful, because it's usually designed to return specific error code directly, and set last error doesn't indicate failure in this case, but instead side effect from underlying code. For you specific case, like I said, I don't see CoTaskMemFree setting error on NULL input, so SHFree won't set it. If your patch still helps without SHFree input pointer checks, the problem is in shlwapi.SHFreeShared instead.
participants (2)
-
Nikolay Sivov -
Olivier F. R. Dierick