[Bug 60346] New: shell32: the window property store returns E_NOTIMPL without initialising its output parameters
http://bugs.winehq.org/show_bug.cgi?id=60346 Bug ID: 60346 Summary: shell32: the window property store returns E_NOTIMPL without initialising its output parameters Product: Wine Version: 11.17 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: shell32 Assignee: wine-bugs@list.winehq.org Reporter: 15959866515@163.com Target Milestone: --- Distribution: --- shell32: the window property store returns E_NOTIMPL without initialising its output parameters SHGetPropertyStoreForWindow() hands out an IPropertyStore whose GetCount, GetAt and GetValue each return E_NOTIMPL without touching the output parameter: static HRESULT WINAPI window_prop_store_GetValue(IPropertyStore *iface, const PROPERTYKEY *key, PROPVARIANT *var) { FIXME("%p, {%s,%lu}, %p\n", iface, debugstr_guid(&key->fmtid), key->pid, var); return E_NOTIMPL; } An out parameter has to be initialised on every path, success or failure. A caller that does not check the HRESULT otherwise reads uninitialised memory, and for a PROPVARIANT whose uninitialised vt happens to read as VT_LPWSTR, pwszVal is an arbitrary pointer that the caller will dereference. The same applies to GetCount's *count and GetAt's *key. Reproduced with a small program that needs nothing but Wine. It calls SHGetPropertyStoreForWindow, poisons the PROPVARIANT with 0xAB and then calls GetValue (attached as propvariant-poison-test.c; build with `x86_64-w64-mingw32-gcc -O2 -o propvariant-poison-test.exe propvariant-poison-test.c -lshell32 -lole32 -luuid`): === SHGetPropertyStoreForWindow === -> 0 store=00007FFFFE8E0E80 === GetValue(PKEY_AppUserModel_ID) with a poisoned PROPVARIANT === GetValue -> 0x80004001 vt = 43947 RESULT: PROPVARIANT WAS NOT TOUCHED -- the bug is present (no string value; pwszVal=ABABABABABABABAB) vt = 43947 = 0xABAB and pwszVal = 0xABABABABABABABAB are the poison, untouched. That is the whole bug: any caller reading pwszVal as a string dereferences 0xABABABABABABABAB. With the attached patch applied, the same binary prints vt = 0 and "PROPVARIANT WAS INITIALISED -- fixed". Full output for both runs is in propvariant-poison-test-output.txt. The proposed fix (propvariant-0001-shell32-init-out-params.patch) initialises the three output parameters before returning E_NOTIMPL. Returning S_OK with an empty variant would be defensible too, but E_NOTIMPL plus initialisation is the smaller behavioural change. One question, which is why this is a bug report rather than a patch with a test: asserting in dlls/shell32/tests that these parameters are initialised on the failure path would need to match what Windows actually does there, and I have no Windows machine to measure it on. If you can say what the expected behaviour is -- or confirm that initialising them is right regardless of what Windows does -- I will send it as a merge request with a test. -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #1 from daoxiang <15959866515@163.com> --- Created attachment 82121 --> http://bugs.winehq.org/attachment.cgi?id=82121 poison test source (needs only mingw + 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #2 from daoxiang <15959866515@163.com> --- Created attachment 82122 --> http://bugs.winehq.org/attachment.cgi?id=82122 poison test output, before and after the 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #3 from daoxiang <15959866515@163.com> --- Created attachment 82123 --> http://bugs.winehq.org/attachment.cgi?id=82123 proposed fix -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 daoxiang <15959866515@163.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |15959866515@163.com -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #4 from Alexandre Julliard <julliard@winehq.org> --- Is there an actual app that calls this function, and if so, what does it need from 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #5 from daoxiang <15959866515@163.com> --- I have the Windows-side results now, measured rather than inferred, and they make this much more concrete. The AUMID path is a dead end: Wine matches Windows there. The window property store does not. Measured on native Windows using GitHub Actions `windows-latest`, with both MSVC and MinGW builds producing the same result, a window with no AppUserModelID set gives: ``` GetValue -> S_OK, *var initialised to VT_EMPTY GetCount -> E_FAIL, *count set to 0 GetAt -> E_FAIL, *key left untouched ``` As a control, after setting a property on the same store, `GetValue` retrieves it (`S_FALSE` in this test, with the value populated). So the store itself is usable; the results above are the behaviour for an absent property. Wine currently returns `E_NOTIMPL` from all three methods and leaves the output untouched. So there are two differences: the HRESULTs and, for `GetValue` and `GetCount`, the output values. The attached patch changes them to match the Windows results: ``` GetCount: *count = 0; return E_FAIL; GetAt: return E_FAIL; GetValue: PropVariantInit(var); return S_OK; ``` The test program used for this (`verify-windows.c`) and its output are attached. It is a plain Win32 program. With MSVC: ``` cl /D_WIN32_WINNT=0x0601 verify-windows.c /link shell32.lib ole32.lib propsys.lib uuid.lib user32.lib ``` or with MinGW: ``` x86_64-w64-mingw32-gcc -O2 -o verify-windows.exe verify-windows.c -lshell32 -lole32 -luuid ``` Running it on Windows prints the results above. Two corrections to my earlier comments: * I suggested following up on an AUMID gap because `GetCurrentApplicationUserModelId` still returns `APPMODEL_ERROR_NO_APPLICATION` after `SetCurrentProcessExplicitAppUserModelID` succeeds. I checked this on Windows, and Windows does the same thing for this classic Win32 process. Wine therefore matches the behaviour I was trying to fix, so I am not proposing any change there. * My first patch initialised the `PROPVARIANT` but still returned `E_NOTIMPL`. That was wrong. Windows returns `S_OK` with `VT_EMPTY` when the property is absent. What this still does not show is an application that is visibly broken by the difference. The original caller I found, NetEase UU Remote, checks the failure and continues normally. So the case for this patch is the Windows/Wine behavioural difference itself, not a known application regression. If that is not enough reason to implement these methods, that's fair. -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #6 from daoxiang <15959866515@163.com> --- Created attachment 82125 --> http://bugs.winehq.org/attachment.cgi?id=82125 the program that produced it; builds with cl or mingw-w64 -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #7 from daoxiang <15959866515@163.com> --- Created attachment 82126 --> http://bugs.winehq.org/attachment.cgi?id=82126 measured output on real Windows (MSVC and MinGW builds agree) -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #8 from daoxiang <15959866515@163.com> --- Created attachment 82127 --> http://bugs.winehq.org/attachment.cgi?id=82127 proposed fix, matching the measured Windows behaviour -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 daoxiang <15959866515@163.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #82123|0 |1 is obsolete| | -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #9 from Alexandre Julliard <julliard@winehq.org> --- (In reply to daoxiang from comment #5)
So the case for this patch is the Windows/Wine behavioural difference itself, not a known application regression. If that is not enough reason to implement these methods, that's fair.
That's not enough reason, no. We only fix behavior differences if they affect a real app. -- 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.
http://bugs.winehq.org/show_bug.cgi?id=60346 --- Comment #10 from daoxiang <15959866515@163.com> --- Fair enough. Thanks for clarifying. I'll leave it here then. -- 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