[Bug 60346] New: shell32: the window property store returns E_NOTIMPL without initialising its output parameters

[WineHQ Bugzilla]

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: [e-mail]
          Reporter: [e-mail]
  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.