[PATCH v2 0/1] MR2141: vbscript: Fix memory leak in owned safearray iterator

[Jason Millard (＠jsm174)]

While running in XCode's profiler, I noticed memory leaks when safearrays were used in `For Each` statements.

The following code would leak a safearray allocation:

```
   For Each obj In vpDict.Keys : Debug.Print "Key" : Next
```

This following code does not leak:

```
   Dim x
   x = vpDict.Keys

   For Each obj In x : Debug.Print "Key" : Next
```

Fixes: https://bugs.winehq.org/show_bug.cgi?id=54456

--
  v2: vbscript: Fix memory leak in owned safearray iterator
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2141

[PATCH v2 1/1] vbscript: Fix memory leak in owned safearray iterator

[Jason Millard]

From: Jason Millard <jsm174(a)gmail.com>

---
 dlls/vbscript/interp.c   | 2 +-
 dlls/vbscript/utils.c    | 9 +++++++--
 dlls/vbscript/vbscript.h | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c
index 4881efa9454..7711dd53baf 100644
--- a/dlls/vbscript/interp.c
+++ b/dlls/vbscript/interp.c
@@ -1487,7 +1487,7 @@ static HRESULT interp_newenum(exec_ctx_t *ctx)
     case VT_VARIANT|VT_ARRAY|VT_BYREF: {
         IEnumVARIANT *iter;
 
-        hres = create_safearray_iter(V_ISBYREF(v.v) ? *V_ARRAYREF(v.v) : V_ARRAY(v.v), &iter);
+        hres = create_safearray_iter(V_ISBYREF(v.v) ? *V_ARRAYREF(v.v) : V_ARRAY(v.v), v.owned, &iter);
         if(FAILED(hres))
             return hres;
 
diff --git a/dlls/vbscript/utils.c b/dlls/vbscript/utils.c
index 03b1abde481..4bfc2c65a0a 100644
--- a/dlls/vbscript/utils.c
+++ b/dlls/vbscript/utils.c
@@ -28,6 +28,7 @@ typedef struct {
     LONG ref;
 
     SAFEARRAY *sa;
+    BOOL owned;
     ULONG i, size;
 } safearray_iter;
 
@@ -75,8 +76,11 @@ static ULONG WINAPI safearray_iter_IEnumVARIANT_Release(IEnumVARIANT *iface)
     TRACE("(%p) ref=%ld\n", iface, ref);
 
     if(!ref) {
-        if(This->sa)
+        if(This->sa) {
             SafeArrayUnlock(This->sa);
+            if(This->owned)
+                SafeArrayDestroy(This->sa);
+	}
         free(This);
     }
 
@@ -160,7 +164,7 @@ static ULONG get_safearray_size(SAFEARRAY *sa)
     return ret;
 }
 
-HRESULT create_safearray_iter(SAFEARRAY *sa, IEnumVARIANT **ev)
+HRESULT create_safearray_iter(SAFEARRAY *sa, BOOL owned, IEnumVARIANT **ev)
 {
     safearray_iter *iter;
     HRESULT hres;
@@ -185,6 +189,7 @@ HRESULT create_safearray_iter(SAFEARRAY *sa, IEnumVARIANT **ev)
     iter->IEnumVARIANT_iface.lpVtbl = &safearray_iter_EnumVARIANTVtbl;
     iter->ref = 1;
     iter->sa = sa;
+    iter->owned = owned;
     iter->i = 0;
     iter->size = get_safearray_size(sa);
 
diff --git a/dlls/vbscript/vbscript.h b/dlls/vbscript/vbscript.h
index 5b0a2222767..13bcf704bf0 100644
--- a/dlls/vbscript/vbscript.h
+++ b/dlls/vbscript/vbscript.h
@@ -415,7 +415,7 @@ BSTR string_replace(BSTR,BSTR,BSTR,int,int,int) DECLSPEC_HIDDEN;
 
 void map_vbs_exception(EXCEPINFO *) DECLSPEC_HIDDEN;
 
-HRESULT create_safearray_iter(SAFEARRAY *sa, IEnumVARIANT **ev) DECLSPEC_HIDDEN;
+HRESULT create_safearray_iter(SAFEARRAY *sa, BOOL owned, IEnumVARIANT **ev) DECLSPEC_HIDDEN;
 
 #define FACILITY_VBS 0xa
 #define MAKE_VBSERROR(code) MAKE_HRESULT(SEVERITY_ERROR, FACILITY_VBS, code)
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/2141

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/vbscript/interp.c:
> case VT_VARIANT|VT_ARRAY|VT_BYREF: {
>          IEnumVARIANT *iter;
>
> -        hres = create_safearray_iter(V_ISBYREF(v.v) ? *V_ARRAYREF(v.v) : V_ARRAY(v.v), &iter);
> +        hres = create_safearray_iter(V_ISBYREF(v.v) ? *V_ARRAYREF(v.v) : V_ARRAY(v.v), v.owned, &iter);

I think we shouldn't destroy the array if it's a reference. Does changing `v.owned` to `v.owned && !V_ISBYREF(v.v)` work for you?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2141#note_23608