[PATCH] user32: Fix NULL dereference in UnregisterDeviceNotification

[Arkadiusz Hiler]

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/bbcd2686d599adf6c5cc8e8466ade0b2c2e5f38a
> https://github.com/ValveSoftware/wine/commit/2d5d0a50652e13254fef3d13df14d3b01efda938

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 at 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._Why.3F
> >
>