[PATCH] user32: Fix NULL dereference in UnregisterDeviceNotification
UnregisterDeviceNotification when provided with NULL should not try to dereference it and just return FALSE. Signed-off-by: Arkadiusz Hiler <arek(a)hiler.eu> --- dlls/sechost/service.c | 3 +++ dlls/user32/tests/Makefile.in | 1 + dlls/user32/tests/misc.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+) create mode 100644 dlls/user32/tests/misc.c diff --git a/dlls/sechost/service.c b/dlls/sechost/service.c index 924a6c9264..68d2b9e78e 100644 --- a/dlls/sechost/service.c +++ b/dlls/sechost/service.c @@ -2109,6 +2109,9 @@ BOOL WINAPI I_ScUnregisterDeviceNotification( HDEVNOTIFY handle ) TRACE("%p\n", handle); + if (!handle) + return FALSE; + EnterCriticalSection( &service_cs ); list_remove( ®istration->entry ); LeaveCriticalSection(&service_cs); diff --git a/dlls/user32/tests/Makefile.in b/dlls/user32/tests/Makefile.in index dd101d69f3..43f843bd09 100644 --- a/dlls/user32/tests/Makefile.in +++ b/dlls/user32/tests/Makefile.in @@ -15,6 +15,7 @@ C_SRCS = \ input.c \ listbox.c \ menu.c \ + misc.c \ monitor.c \ msg.c \ resource.c \ diff --git a/dlls/user32/tests/misc.c b/dlls/user32/tests/misc.c new file mode 100644 index 0000000000..1f55a65a3e --- /dev/null +++ b/dlls/user32/tests/misc.c @@ -0,0 +1,33 @@ +/* + * Unit test suite for misc functions. + * + * Copyright 2020 Arkadiusz Hiler + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include "wine/test.h" +#include "winuser.h" + +static void test_UnregisterDeviceNotification(void) +{ + BOOL ret = UnregisterDeviceNotification(NULL); + ok(ret == FALSE, "Unregistering NULL Device Notification returned: %d\n", ret); +} + +START_TEST(misc) +{ + test_UnregisterDeviceNotification(); +} -- 2.26.2
On Sat, May 16, 2020 at 04:28:48PM +0300, Arkadiusz Hiler wrote:
UnregisterDeviceNotification when provided with NULL should not try to dereference it and just return FALSE.
Signed-off-by: Arkadiusz Hiler <arek(a)hiler.eu>
Hey folks, anything wrong with the patch that would stop you from picking it up? The fix is fairly simple and comes with a test. The testbot seems to be content with it[0]. I also don't see anything obviously incorrect upon second and third look, so I'll try persistence[1] :-) [0]: https://testbot.winehq.org/JobDetails.pl?Key=71872 [1]: https://wiki.winehq.org/Developer_FAQ#I_sent_a_patch.2C_but_it_got_ignored._... -- Cheers, Arek
Hi Arkadiusz, Checking NULL is usually fine. But I think you should add the tests to sechost. There are also https://github.com/ValveSoftware/wine/commit/bbcd2686d599adf6c5cc8e8466ade0b... https://github.com/ValveSoftware/wine/commit/2d5d0a50652e13254fef3d13df14d3b... I haven't look at if I_ScUnregisterDeviceNotification() is already fully implemented. But if it is not, it would be more useful to get it upstream rather than simply checking NULL. If checking NULL is enough to fix a real world application and you think upstreamming the work is too much for you then a simple NULL fix is also fine. Thanks, Zhiyi On 5/20/20 6:30 PM, Arkadiusz Hiler wrote:
On Sat, May 16, 2020 at 04:28:48PM +0300, Arkadiusz Hiler wrote:
UnregisterDeviceNotification when provided with NULL should not try to dereference it and just return FALSE.
Signed-off-by: Arkadiusz Hiler <arek(a)hiler.eu> Hey folks,
anything wrong with the patch that would stop you from picking it up?
The fix is fairly simple and comes with a test. The testbot seems to be content with it[0]. I also don't see anything obviously incorrect upon second and third look, so I'll try persistence[1] :-)
[0]: https://testbot.winehq.org/JobDetails.pl?Key=71872 [1]: https://wiki.winehq.org/Developer_FAQ#I_sent_a_patch.2C_but_it_got_ignored._...
On Wed, May 20, 2020 at 08:14:57PM +0800, Zhiyi Zhang wrote:
Hi Arkadiusz,
Checking NULL is usually fine. But I think you should add the tests to sechost.
Heyo, I am not sure how to get it tested there. The functions doesn't seem to be exposed by any wine/Windows headers. There's also no existing tests for it. With user32.dll is was straightforward because we have winuser.h. I have seen the exact call (UnregisterDeviceNotification(NULL)) crashing a real .exe with wine and have seen no similar complains from Windows users, so that's where I have started.
There are also https://github.com/ValveSoftware/wine/commit/bbcd2686d599adf6c5cc8e8466ade0b... https://github.com/ValveSoftware/wine/commit/2d5d0a50652e13254fef3d13df14d3b...
Good point. I guess I will have to do all my testing against winehq's wine, Valve's wine and wine-staging now to see what's already a WIP. Developer FAQ could benefit from a note on this.
I haven't look at if I_ScUnregisterDeviceNotification() is already fully implemented. But if it is not, it would be more useful to get it upstream rather than simply checking NULL. If checking NULL is enough to fix a real world application and you think upstreamming the work is too much for you then a simple NULL fix is also fine.
I admit that I have taken a very simplistic approach here. I have seen an .exe exploding under wine, rootcaused it to this NULL dereference and tried to fix the crash alone with not much interest in getting full implementation in place yet. In the process we have got a new conformance test that confirms that UnregisterDeviceNotification(NULL) just returns FALSE under Windows. I just took "send a small, very clean patch first" to heart and considered looking more into getting that .exe more functional over time.
Thanks, Zhiyi
On 5/20/20 6:30 PM, Arkadiusz Hiler wrote:
On Sat, May 16, 2020 at 04:28:48PM +0300, Arkadiusz Hiler wrote:
UnregisterDeviceNotification when provided with NULL should not try to dereference it and just return FALSE.
Signed-off-by: Arkadiusz Hiler <arek(a)hiler.eu> Hey folks,
anything wrong with the patch that would stop you from picking it up?
The fix is fairly simple and comes with a test. The testbot seems to be content with it[0]. I also don't see anything obviously incorrect upon second and third look, so I'll try persistence[1] :-)
[0]: https://testbot.winehq.org/JobDetails.pl?Key=71872 [1]: https://wiki.winehq.org/Developer_FAQ#I_sent_a_patch.2C_but_it_got_ignored._...
On 5/20/20 7:14 AM, Zhiyi Zhang wrote:
Hi Arkadiusz,
Checking NULL is usually fine. But I think you should add the tests to sechost. There are also https://github.com/ValveSoftware/wine/commit/bbcd2686d599adf6c5cc8e8466ade0b... https://github.com/ValveSoftware/wine/commit/2d5d0a50652e13254fef3d13df14d3b...
I haven't look at if I_ScUnregisterDeviceNotification() is already fully implemented. But if it is not, it would be more useful to get it upstream rather than simply checking NULL. If checking NULL is enough to fix a real world application and you think upstreamming the work is too much for you then a simple NULL fix is also fine.
Personally, I think the test makes sense where it is. On the other hand, it'd be nice to mention in the commit message what application is fixed by this.
Hello, I have the same fix in my repo and finally spared some time to fill a bug report: https://bugs.winehq.org/show_bug.cgi?id=49211 Regards Roman Dne 20. 05. 20 v 16:01 Zebediah Figura napsal(a):
On 5/20/20 7:14 AM, Zhiyi Zhang wrote:
Hi Arkadiusz,
Checking NULL is usually fine. But I think you should add the tests to sechost. There are also https://github.com/ValveSoftware/wine/commit/bbcd2686d599adf6c5cc8e8466ade0b...
https://github.com/ValveSoftware/wine/commit/2d5d0a50652e13254fef3d13df14d3b...
I haven't look at if I_ScUnregisterDeviceNotification() is already fully implemented. But if it is not, it would be more useful to get it upstream rather than simply checking NULL. If checking NULL is enough to fix a real world application and you think upstreamming the work is too much for you then a simple NULL fix is also fine.
Personally, I think the test makes sense where it is.
On the other hand, it'd be nice to mention in the commit message what application is fixed by this.
On Wed, May 20, 2020 at 09:01:21AM -0500, Zebediah Figura wrote:
On 5/20/20 7:14 AM, Zhiyi Zhang wrote:
Hi Arkadiusz,
Checking NULL is usually fine. But I think you should add the tests to sechost. There are also https://github.com/ValveSoftware/wine/commit/bbcd2686d599adf6c5cc8e8466ade0b... https://github.com/ValveSoftware/wine/commit/2d5d0a50652e13254fef3d13df14d3b...
I haven't look at if I_ScUnregisterDeviceNotification() is already fully implemented. But if it is not, it would be more useful to get it upstream rather than simply checking NULL. If checking NULL is enough to fix a real world application and you think upstreamming the work is too much for you then a simple NULL fix is also fine.
Personally, I think the test makes sense where it is.
Hey, Sorry for late reply, I subscribe to about a dozen of mailing lists and I am used that people in the discussion are explicitly CCed. I'll adjust my filtering for wine-devel. I am still wrapping my head around some of the things here. As far as I undersand I_ScUnregisterDeviceNotification() is internal and is not exposed by any header. UnregisterDeviceNotification() it the call that a lot of programs are usining through the winuser.h, and therefore should have conformance tests on it's own. I_ScUnregisterDeviceNotification() may be called by some .exes though anyway, so it should have it's own set of conformance tests too. Is this correct?
On the other hand, it'd be nice to mention in the commit message what application is fixed by this.
It's the Glorious Model O control software. It won't work but at least this fixes the crash. I'll resend my patch with that mentioned + Roman's bug linked and CC you. Thanks! -- Cheers, Arek
participants (4)
-
Arkadiusz Hiler -
Roman Pišl -
Zebediah Figura -
Zhiyi Zhang