[PATCH 0/2] MR10978: scrrun/dictionary: Report key-hashing failures from lookups.
An object key that can no longer be hashed is reported as absent by Exists and not-found by Remove, instead of surfacing the hashing error like native — verified across all six key-taking methods. Method Native Wine pre-fix Wine post-fix ----------- ------------ ------------------------------- ------------ Add 0x800a0005 0x800a0005 (ok) 0x800a0005 Item read 0x800a0005 0x800a0005 (ok) 0x800a0005 Item write 0x800a0005 0x800a0005 (ok) 0x800a0005 Key rename 0x800a0005 S_OK, created bogus key (DIFF) 0x800a0005 Exists 0x800a0005 S_OK / False (DIFF) 0x800a0005 Remove 0x800a0005 0x800a802b (not-found) (DIFF) 0x800a0005 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10978
From: Francis De Brabandere <francisdb@gmail.com> When an object used as a dictionary key can no longer be hashed (its QueryInterface for IID_IUnknown fails), Exists and Remove should surface that failure as CTL_E_ILLEGALFUNCTIONCALL rather than treating the key as absent. --- dlls/scrrun/tests/dictionary.c | 85 ++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/dlls/scrrun/tests/dictionary.c b/dlls/scrrun/tests/dictionary.c index e1cb33c45a7..7fbaaa91001 100644 --- a/dlls/scrrun/tests/dictionary.c +++ b/dlls/scrrun/tests/dictionary.c @@ -326,6 +326,90 @@ static IUnknown test_unk = { &test_unk_vtbl }; static IUnknown test_unk2 = { &test_unk_no_vtbl }; static IDispatch test_disp = { &test_disp_vtbl }; +/* A dispatch object whose QueryInterface(IID_IUnknown) can be made to fail, + * modelling a COM object that has been logically destroyed while still + * referenced as a dictionary key. */ +struct degen_disp +{ + IDispatch IDispatch_iface; + BOOL qi_unknown_fails; +}; + +static HRESULT WINAPI degen_QI(IDispatch *iface, REFIID riid, void **obj) +{ + struct degen_disp *d = CONTAINING_RECORD(iface, struct degen_disp, IDispatch_iface); + + if (IsEqualIID(riid, &IID_IDispatch)) { + *obj = iface; + return S_OK; + } + if (IsEqualIID(riid, &IID_IUnknown)) { + if (d->qi_unknown_fails) { + *obj = NULL; + return E_NOINTERFACE; + } + *obj = iface; + return S_OK; + } + *obj = NULL; + return E_NOINTERFACE; +} + +static ULONG WINAPI degen_AddRef(IDispatch *iface) { return 2; } +static ULONG WINAPI degen_Release(IDispatch *iface) { return 1; } + +static const IDispatchVtbl degen_disp_vtbl = { + degen_QI, + degen_AddRef, + degen_Release, + test_disp_GetTypeInfoCount, + test_disp_GetTypeInfo, + test_disp_GetIDsOfNames, + test_disp_Invoke +}; + +static void test_object_key_hashfail(void) +{ + struct degen_disp obj = { { °en_disp_vtbl }, FALSE }; + VARIANT_BOOL exists; + IDictionary *dict; + VARIANT key, item; + HRESULT hr; + + hr = CoCreateInstance(&CLSID_Dictionary, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER, + &IID_IDictionary, (void**)&dict); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + V_VT(&key) = VT_DISPATCH; + V_DISPATCH(&key) = &obj.IDispatch_iface; + V_VT(&item) = VT_I4; + V_I4(&item) = 42; + hr = IDictionary_Add(dict, &key, &item); + ok(hr == S_OK, "Add: %#lx.\n", hr); + + exists = VARIANT_FALSE; + hr = IDictionary_Exists(dict, &key, &exists); + ok(hr == S_OK, "Exists (live): %#lx.\n", hr); + ok(exists == VARIANT_TRUE, "expected the key to exist\n"); + + /* The object is now degenerate: hashing the key fails. Exists and Remove + * must surface that failure rather than report the key as absent. */ + obj.qi_unknown_fails = TRUE; + + hr = IDictionary_Exists(dict, &key, &exists); + todo_wine ok(hr == CTL_E_ILLEGALFUNCTIONCALL, "Exists (degenerate): %#lx.\n", hr); + + hr = IDictionary_Remove(dict, &key); + todo_wine ok(hr == CTL_E_ILLEGALFUNCTIONCALL, "Remove (degenerate): %#lx.\n", hr); + + /* Restore the object and confirm the key is still present and removable. */ + obj.qi_unknown_fails = FALSE; + hr = IDictionary_Remove(dict, &key); + ok(hr == S_OK, "Remove (restored): %#lx.\n", hr); + + IDictionary_Release(dict); +} + static void test_hash_value(void) { /* string test data */ @@ -1230,6 +1314,7 @@ START_TEST(dictionary) test_Remove(); test_Item(); test_Add(); + test_object_key_hashfail(); test_IEnumVARIANT(); test_putref_Item(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10978
From: Francis De Brabandere <francisdb@gmail.com> get_keyitem_pair() returned NULL both when a key was absent and when its hash could not be computed, so Exists reported a present-but-unhashable object key as absent and Remove failed with element-not-found instead of the hashing error. Return the HashVal failure to callers so Exists, Remove, Item, Add, and Key all surface CTL_E_ILLEGALFUNCTIONCALL for a degenerate object key, matching native. --- dlls/scrrun/dictionary.c | 59 ++++++++++++++++++++++++++-------- dlls/scrrun/tests/dictionary.c | 4 +-- 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/dlls/scrrun/dictionary.c b/dlls/scrrun/dictionary.c index af771c9245a..1b618865360 100644 --- a/dlls/scrrun/dictionary.c +++ b/dlls/scrrun/dictionary.c @@ -199,24 +199,30 @@ static BOOL is_matching_key(const struct dictionary *dict, const struct keyitem_ } } -static struct keyitem_pair *get_keyitem_pair(struct dictionary *dict, VARIANT *key) +/* Looks up the pair for a key. On success *ret is the matching pair, or NULL + if the key is not present. A failure to hash the key (e.g. an object key + that can no longer be queried for IID_IUnknown) is reported to the caller + rather than being mistaken for an absent key. */ +static HRESULT get_keyitem_pair(struct dictionary *dict, VARIANT *key, struct keyitem_pair **ret) { struct keyitem_pair *pair; struct list *head, *entry; VARIANT hash, v; HRESULT hr; + *ret = NULL; + hr = IDictionary_get_HashVal(&dict->IDictionary_iface, key, &hash); if (FAILED(hr)) - return NULL; + return hr; head = get_bucket_head(dict, V_I4(&hash)); if (!head->next || list_empty(head)) - return NULL; + return S_OK; VariantInit(&v); - if (FAILED(VariantCopyInd(&v, key))) - return NULL; + if (FAILED(hr = VariantCopyInd(&v, key))) + return hr; entry = list_head(head); do @@ -225,13 +231,14 @@ static struct keyitem_pair *get_keyitem_pair(struct dictionary *dict, VARIANT *k if (is_matching_key(dict, pair, &v, V_I4(&hash))) { VariantClear(&v); - return pair; + *ret = pair; + return S_OK; } } while ((entry = list_next(head, entry))); VariantClear(&v); - return NULL; + return S_OK; } static HRESULT add_keyitem_pair(struct dictionary *dict, VARIANT *key, VARIANT *item) @@ -563,10 +570,13 @@ static HRESULT WINAPI dictionary_putref_Item(IDictionary *iface, VARIANT *key, V { struct dictionary *dictionary = impl_from_IDictionary(iface); struct keyitem_pair *pair; + HRESULT hr; TRACE("%p, %s, %s.\n", iface, debugstr_variant(key), debugstr_variant(item)); - if ((pair = get_keyitem_pair(dictionary, key))) + if (FAILED(hr = get_keyitem_pair(dictionary, key, &pair))) + return hr; + if (pair) return VariantCopyInd(&pair->item, item); return add_keyitem_pair(dictionary, key, item); @@ -576,10 +586,13 @@ static HRESULT WINAPI dictionary_put_Item(IDictionary *iface, VARIANT *key, VARI { struct dictionary *dictionary = impl_from_IDictionary(iface); struct keyitem_pair *pair; + HRESULT hr; TRACE("%p, %s, %s.\n", iface, debugstr_variant(key), debugstr_variant(item)); - if ((pair = get_keyitem_pair(dictionary, key))) + if (FAILED(hr = get_keyitem_pair(dictionary, key, &pair))) + return hr; + if (pair) return VariantCopyInd(&pair->item, item); return add_keyitem_pair(dictionary, key, item); @@ -589,10 +602,13 @@ static HRESULT WINAPI dictionary_get_Item(IDictionary *iface, VARIANT *key, VARI { struct dictionary *dictionary = impl_from_IDictionary(iface); struct keyitem_pair *pair; + HRESULT hr; TRACE("%p, %s, %p.\n", iface, debugstr_variant(key), item); - if ((pair = get_keyitem_pair(dictionary, key))) + if (FAILED(hr = get_keyitem_pair(dictionary, key, &pair))) + return hr; + if (pair) VariantCopy(item, &pair->item); else { VariantInit(item); @@ -605,10 +621,14 @@ static HRESULT WINAPI dictionary_get_Item(IDictionary *iface, VARIANT *key, VARI static HRESULT WINAPI dictionary_Add(IDictionary *iface, VARIANT *key, VARIANT *item) { struct dictionary *dictionary = impl_from_IDictionary(iface); + struct keyitem_pair *pair; + HRESULT hr; TRACE("%p, %s, %s.\n", iface, debugstr_variant(key), debugstr_variant(item)); - if (get_keyitem_pair(dictionary, key)) + if (FAILED(hr = get_keyitem_pair(dictionary, key, &pair))) + return hr; + if (pair) return CTL_E_KEY_ALREADY_EXISTS; return add_keyitem_pair(dictionary, key, item); @@ -627,13 +647,18 @@ static HRESULT WINAPI dictionary_get_Count(IDictionary *iface, LONG *count) static HRESULT WINAPI dictionary_Exists(IDictionary *iface, VARIANT *key, VARIANT_BOOL *exists) { struct dictionary *dictionary = impl_from_IDictionary(iface); + struct keyitem_pair *pair; + HRESULT hr; TRACE("%p, %s, %p.\n", iface, debugstr_variant(key), exists); if (!exists) return CTL_E_ILLEGALFUNCTIONCALL; - *exists = get_keyitem_pair(dictionary, key) != NULL ? VARIANT_TRUE : VARIANT_FALSE; + if (FAILED(hr = get_keyitem_pair(dictionary, key, &pair))) + return hr; + + *exists = pair ? VARIANT_TRUE : VARIANT_FALSE; return S_OK; } @@ -686,7 +711,10 @@ static HRESULT WINAPI dictionary_put_Key(IDictionary *iface, VARIANT *key, VARIA TRACE("%p, %s, %s.\n", iface, debugstr_variant(key), debugstr_variant(newkey)); - if ((pair = get_keyitem_pair(dictionary, key))) + if (FAILED(hr = get_keyitem_pair(dictionary, key, &pair))) + return hr; + + if (pair) { /* found existing pair for a key, add new pair with new key and old item and remove old pair after that */ @@ -746,10 +774,13 @@ static HRESULT WINAPI dictionary_Remove(IDictionary *iface, VARIANT *key) { struct dictionary *dictionary = impl_from_IDictionary(iface); struct keyitem_pair *pair; + HRESULT hr; TRACE("%p, %s.\n", iface, debugstr_variant(key)); - if (!(pair = get_keyitem_pair(dictionary, key))) + if (FAILED(hr = get_keyitem_pair(dictionary, key, &pair))) + return hr; + if (!pair) return CTL_E_ELEMENT_NOT_FOUND; notify_remove_pair(&dictionary->notifier, &pair->entry); diff --git a/dlls/scrrun/tests/dictionary.c b/dlls/scrrun/tests/dictionary.c index 7fbaaa91001..8fc369e7062 100644 --- a/dlls/scrrun/tests/dictionary.c +++ b/dlls/scrrun/tests/dictionary.c @@ -397,10 +397,10 @@ static void test_object_key_hashfail(void) obj.qi_unknown_fails = TRUE; hr = IDictionary_Exists(dict, &key, &exists); - todo_wine ok(hr == CTL_E_ILLEGALFUNCTIONCALL, "Exists (degenerate): %#lx.\n", hr); + ok(hr == CTL_E_ILLEGALFUNCTIONCALL, "Exists (degenerate): %#lx.\n", hr); hr = IDictionary_Remove(dict, &key); - todo_wine ok(hr == CTL_E_ILLEGALFUNCTIONCALL, "Remove (degenerate): %#lx.\n", hr); + ok(hr == CTL_E_ILLEGALFUNCTIONCALL, "Remove (degenerate): %#lx.\n", hr); /* Restore the object and confirm the key is still present and removable. */ obj.qi_unknown_fails = FALSE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10978
And yes this is something we hit in the wild. Visual Pinball scripts keep track of balls in play. Sometimes these balls are destroyed but kept in a Dictionary. Correct errors would have helped us track this down. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10978#note_141036
This merge request was approved by Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10978
participants (3)
-
Francis De Brabandere -
Francis De Brabandere (＠francisdb)
-
Nikolay Sivov (＠nsivov)