[PATCH v2 1/2] jscript: Only remove one reference to each entry when clearing the Map or Set.

[Gabriel Ivăncescu]

Checking for list_empty doesn't stop until all entries have been removed,
no matter their ref count.

Signed-off-by: Gabriel Ivăncescu 
---

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;

[PATCH v2 2/2] jscript: Fix iterating through the map while entries are removed.

[Gabriel Ivăncescu]

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 
---

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;

[PATCH v2 2/2] jscript: Fix iterating through the map while entries are removed.

[Jacek Caban]

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
>
> 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