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@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] :-)