[Bug 48941] New: Wine's implementation of IMalloc.DidAlloc relies on a spy mechanism, while Windows just calls "HeapValidate"
https://bugs.winehq.org/show_bug.cgi?id=48941 Bug ID: 48941 Summary: Wine's implementation of IMalloc.DidAlloc relies on a spy mechanism, while Windows just calls "HeapValidate" Product: Wine Version: unspecified Hardware: x86 OS: other Status: UNCONFIRMED Severity: major Priority: P2 Component: -unknown Assignee: wine-bugs(a)winehq.org Reporter: contact(a)kcsoftwares.com Created attachment 66916 --> https://bugs.winehq.org/attachment.cgi?id=66916 Executable allowing to raise the problem. When using this test application : https://jira.reactos.org/secure/attachment/56514/ros.exe initially during ReactOS tests using latest development version, the problem described in https://jira.reactos.org/browse/CORE-15262 has been observed. Investigation by the ReactOS dev team, pointed out that the root cause was on Wine side because Wine's implementation of IMalloc.DidAlloc relies on a spy mechanism, while Windows just calls "HeapValidate". See comments on Wine's implementation of IMalloc.DidAlloc relies on a spy mechanism, while Windows just calls "HeapValidate" for further details. Reproduce : Open ros.exe attachment Click on button "once", select a folder : Pop-up showing the folder path Click again on button => nothing happens due to the bug in Wine's IMalloc.DidAlloc while this works perfectly in Windows. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 Nikolay Sivov <bunglehead(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |normal --- Comment #1 from Nikolay Sivov <bunglehead(a)gmail.com> --- It's true that what we have is incomplete, for example fSpyed is never set, but how do you that Windows is using HeapValidate and when? Also please attach source code for that test case. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #2 from Kyle_Katarn <contact(a)kcsoftwares.com> --- This is the code section from JCL library JclShell.pas causing the problem : function PidlFree(var IdList: PItemIdList): Boolean; var Malloc: IMalloc; begin Result := False; if IdList = nil then Result := True else begin Malloc := nil; if Succeeded(SHGetMalloc(Malloc)) and (Malloc.DidAlloc(IdList) > 0) then begin Malloc.Free(IdList); IdList := nil; Result := True; end; end; end; -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #3 from Kyle_Katarn <contact(a)kcsoftwares.com> --- (In reply to Nikolay Sivov from comment #1)
It's true that what we have is incomplete, for example fSpyed is never set, but how do you that Windows is using HeapValidate and when? Also please attach source code for that test case.
Do you need more details ? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #4 from Nikolay Sivov <bunglehead(a)gmail.com> --- (In reply to Kyle_Katarn from comment #3)
(In reply to Nikolay Sivov from comment #1)
It's true that what we have is incomplete, for example fSpyed is never set, but how do you that Windows is using HeapValidate and when? Also please attach source code for that test case.
Do you need more details ?
Question regarding HeapValidate() still stands. For use case, no, it's clear what breaks - we simply don't return correct flags, with spy or without. Of course there is not reason to do DidAlloc, or SHGetMalloc() in this code, it should simply call CoTaskMemFree(), or ILFree/SHFree() which is the same thing. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #5 from Kyle_Katarn <contact(a)kcsoftwares.com> --- Thanks ! I opened a ticket on JCL side. Would you have a patch to propose to change the code according to your suggestions ? Regarding Wine, do you imply that the fix in order to correctly handle such code (which runs perfectly in Windows) will not be investigated ? Or do you have a potential fix in Wine to suggest ? this is blocking all apps using this JCL code section in Wine (+ potentially others). Thanks ! -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #6 from Kyle_Katarn <contact(a)kcsoftwares.com> --- https://issuetracker.delphi-jedi.org/view.php?id=6688 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #7 from Nikolay Sivov <bunglehead(a)gmail.com> --- (In reply to Kyle_Katarn from comment #5)
Thanks ! I opened a ticket on JCL side. Would you have a patch to propose to change the code according to your suggestions ?
I'm not going to patch it, no, and I don't know what was authors motivation too. If it was a workaround for some garbage pointer passed in, that could cause damage. Ideally callers should not pass invalid input, and if it's some internal function, callers should be fixed, and function could then be simplified.
Regarding Wine, do you imply that the fix in order to correctly handle such code (which runs perfectly in Windows) will not be investigated ? Or do you have a potential fix in Wine to suggest ?
Sure, it should be fixed in Wine too. I'll probably take a look.
this is blocking all apps using this JCL code section in Wine (+ potentially others). Thanks !
-- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 Nikolay Sivov <bunglehead(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|-unknown |ole32 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #8 from Kyle_Katarn <contact(a)kcsoftwares.com> --- OK. Issue reported to JCL dev team. Very unlikely to see a modification coming as the project seems "on hold" and no longer as active as it used to be. Regarding issue / impact, the problem impacting execution in Wine is of course happening even on fully nominal cases. Thank you for giving a look at how Wine could be improved with regard to this. Once implemented, please let me know and i'll pass the information to the ReactOS dev team too ! -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 Nikolay Sivov <bunglehead(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Wine's implementation of |IMalloc::DidAlloc() return |IMalloc.DidAlloc relies on |value is inaccurate |a spy mechanism, while | |Windows just calls | |"HeapValidate" | -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #9 from Nikolay Sivov <bunglehead(a)gmail.com> --- This patch seems enough https://source.winehq.org/git/wine.git/?a=commit;h=08f4b6ee0a05fe27e56cb9777.... Please retest with current wine. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 Nikolay Sivov <bunglehead(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |5.6 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #10 from Kyle_Katarn <contact(a)kcsoftwares.com> --- Thanks ! I'll check with ReactOS team ! -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #11 from Kyle_Katarn <contact(a)kcsoftwares.com> --- (In reply to Nikolay Sivov from comment #9)
This patch seems enough https://source.winehq.org/git/wine.git/?a=commit; h=08f4b6ee0a05fe27e56cb9777dce845dcf1072f8. Please retest with current wine.
Hello, Thank you very much. Positive results from the preliminary test done by the ReactOS team on the test case + more complex cases (based SUMo). We'll continue tests, but this looks very good. In which Wine version do you plan to integrate this patch ? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #12 from Nikolay Sivov <bunglehead(a)gmail.com> --- Wine 5.7 will be first release to include it. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #13 from Kyle_Katarn <contact(a)kcsoftwares.com> --- (In reply to Nikolay Sivov from comment #12)
Wine 5.7 will be first release to include it.
Perfect, thanks ! -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 Nikolay Sivov <bunglehead(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|UNCONFIRMED |RESOLVED --- Comment #14 from Nikolay Sivov <bunglehead(a)gmail.com> --- Marking fixed as test application works now, commit 08f4b6ee0a05fe27e56cb9777dce845dcf1072f8. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #15 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 5.7. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 --- Comment #16 from Kyle_Katarn <contact(a)kcsoftwares.com> --- (In reply to Alexandre Julliard from comment #15)
Closing bugs fixed in 5.7.
Thank you -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48941 Anastasius Focht <focht(a)gmx.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |08f4b6ee0a05fe27e56cb9777dc | |e845dcf1072f8 CC| |focht(a)gmx.net Keywords| |download -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla