[PATCH v2 1/2] jscript: Only remove one reference to each entry when clearing the Map or Set.
Checking for list_empty doesn't stop until all entries have been removed, no matter their ref count. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v2: Also fix it for Set... dlls/jscript/set.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index d1ea663..0ef014b 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -223,6 +223,7 @@ static HRESULT iterate_map(MapInstance *map, script_ctx_t *ctx, unsigned argc, j static HRESULT Map_clear(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { + struct jsval_map_entry *entry, *entry2; MapInstance *map; HRESULT hres; @@ -232,10 +233,8 @@ static HRESULT Map_clear(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned TRACE("%p\n", map); - while(!list_empty(&map->entries)) { - struct jsval_map_entry *entry = LIST_ENTRY(list_head(&map->entries), struct jsval_map_entry, list_entry); + LIST_FOR_EACH_ENTRY_SAFE(entry, entry2, &map->entries, struct jsval_map_entry, list_entry) delete_map_entry(map, entry); - } if(r) *r = jsval_undefined(); return S_OK; @@ -443,6 +442,7 @@ static HRESULT Set_add(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned ar static HRESULT Set_clear(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { + struct jsval_map_entry *entry, *entry2; MapInstance *set; HRESULT hres; @@ -452,10 +452,8 @@ static HRESULT Set_clear(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned TRACE("%p\n", set); - while(!list_empty(&set->entries)) { - struct jsval_map_entry *entry = LIST_ENTRY(list_head(&set->entries), struct jsval_map_entry, list_entry); + LIST_FOR_EACH_ENTRY_SAFE(entry, entry2, &set->entries, struct jsval_map_entry, list_entry) delete_map_entry(set, entry); - } if(r) *r = jsval_undefined(); return S_OK; -- 2.34.1
The current entry as well as the next entry may both be removed while iterating. Since we release the entry after the callback, obtaining the "next" entry would be using possibly freed memory. A safe iteration is required, but we need to obtain the next entry after the callback, not at the start of the loop, since it can be removed during it. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This is a pre-existing problem. See for example 64-bit Debian VM results here: https://testbot.winehq.org/JobDetails.pl?Key=116313#k2202 dlls/jscript/set.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index 0ef014b..1d2ced3 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -183,7 +183,7 @@ static HRESULT set_map_entry(MapInstance *map, jsval_t key, jsval_t value, jsval static HRESULT iterate_map(MapInstance *map, script_ctx_t *ctx, unsigned argc, jsval_t *argv, jsval_t *r) { - struct jsval_map_entry *entry; + struct jsval_map_entry *entry, *entry2; IDispatch *context_obj = NULL; HRESULT hres; @@ -200,7 +200,7 @@ static HRESULT iterate_map(MapInstance *map, script_ctx_t *ctx, unsigned argc, j context_obj = get_object(argv[1]); } - LIST_FOR_EACH_ENTRY(entry, &map->entries, struct jsval_map_entry, list_entry) { + LIST_FOR_EACH_ENTRY_SAFE(entry, entry2, &map->entries, struct jsval_map_entry, list_entry) { jsval_t args[3], v; if(entry->deleted) continue; @@ -210,6 +210,9 @@ static HRESULT iterate_map(MapInstance *map, script_ctx_t *ctx, unsigned argc, j grab_map_entry(entry); hres = disp_call_value(ctx, get_object(argv[0]), context_obj, DISPATCH_METHOD, ARRAY_SIZE(args), args, &v); + + /* The callback may have possibly removed the next entry, so grab it here */ + entry2 = LIST_ENTRY(entry->list_entry.next, struct jsval_map_entry, list_entry); release_map_entry(entry); if(FAILED(hres)) return hres; -- 2.34.1
Hi Gabriel, On 6/6/22 16:10, Gabriel Ivăncescu wrote:
The current entry as well as the next entry may both be removed while iterating. Since we release the entry after the callback, obtaining the "next" entry would be using possibly freed memory. A safe iteration is required, but we need to obtain the next entry after the callback, not at the start of the loop, since it can be removed during it.
Signed-off-by: Gabriel Ivăncescu<gabrielopcode(a)gmail.com> ---
This is a pre-existing problem. See for example 64-bit Debian VM results here:https://testbot.winehq.org/JobDetails.pl?Key=116313#k2202
dlls/jscript/set.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
I wrote a test and resubmitted with a cleaner version of the second patch. Thanks, Jacek
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban