[PATCH v3 0/1] MR2132: vbscript: Fix memory leak in interp_redim_preserve

[Jason Millard (＠jsm174)]

While running in XCode's profiler, I noticed two memory leaks in `interp_redim_preserve`.

After looking at `interp_redim`, the `bounds` structure is freed.

I've updated `interp_redim_preserve` to free `bounds` when the array is NULL and not NULL.

--
  v3: vbscript: Fix memory leak in interp_redim_preserve

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

[PATCH v3 1/1] vbscript: Fix memory leak in interp_redim_preserve

[Jason Millard]

From: Jason Millard

---
 dlls/vbscript/interp.c | 48 ++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c
index b3be3bea6cf..b22dfb22b20 100644
--- a/dlls/vbscript/interp.c
+++ b/dlls/vbscript/interp.c
@@ -1369,30 +1369,36 @@ static HRESULT interp_redim_preserve(exec_ctx_t *ctx)
     array = V_ISBYREF(v) ? *V_ARRAYREF(v) : V_ARRAY(v);

hres = array_bounds_from_stack(ctx, dim_cnt, &bounds);
-    if(FAILED(hres))
-        return hres;
-
-    if(array == NULL || array->cDims == 0) {
-        /* can initially allocate the array */
-        array = SafeArrayCreate(VT_VARIANT, dim_cnt, bounds);
-        VariantClear(v);
-        V_VT(v) = VT_ARRAY|VT_VARIANT;
-        V_ARRAY(v) = array;
-        return S_OK;
-    } else if(array->cDims != dim_cnt) {
-        /* can't otherwise change the number of dimensions */
-        TRACE("Can't resize %s, cDims %d != %d\n", debugstr_w(identifier), array->cDims, dim_cnt);
-        return MAKE_VBSERROR(VBSE_OUT_OF_BOUNDS);
-    } else {
-        /* can resize the last dimensions (if others match */
-        for(i = 0; i+1 < dim_cnt; ++i) {
-            if(array->rgsabound[array->cDims - 1 - i].cElements != bounds[i].cElements) {
-                TRACE("Can't resize %s, bound[%d] %ld != %ld\n", debugstr_w(identifier), i, array->rgsabound[i].cElements, bounds[i].cElements);
-                return MAKE_VBSERROR(VBSE_OUT_OF_BOUNDS);
+    if(SUCCEEDED(hres)) {
+        if(array == NULL || array->cDims == 0) {
+            /* can initially allocate the array */
+            array = SafeArrayCreate(VT_VARIANT, dim_cnt, bounds);
+            if(!array)
+                hres = E_OUTOFMEMORY;
+            if(SUCCEEDED(hres)) {
+                VariantClear(v);
+                V_VT(v) = VT_ARRAY|VT_VARIANT;
+                V_ARRAY(v) = array;
+            }
+        } else if(array->cDims != dim_cnt) {
+            /* can't otherwise change the number of dimensions */
+            TRACE("Can't resize %s, cDims %d != %d\n", debugstr_w(identifier), array->cDims, dim_cnt);
+            hres = MAKE_VBSERROR(VBSE_OUT_OF_BOUNDS);
+        } else {
+            /* can resize the last dimensions (if others match */
+            for(i = 0; i+1 < dim_cnt; ++i) {
+                if(array->rgsabound[array->cDims - 1 - i].cElements != bounds[i].cElements) {
+                    TRACE("Can't resize %s, bound[%d] %ld != %ld\n", debugstr_w(identifier), i, array->rgsabound[i].cElements, bounds[i].cElements);
+                    hres = MAKE_VBSERROR(VBSE_OUT_OF_BOUNDS);
+		    break;
+                }
             }
+            if(SUCCEEDED(hres))
+                hres = SafeArrayRedim(array, &bounds[dim_cnt-1]);
         }
-        return SafeArrayRedim(array, &bounds[dim_cnt-1]);
     }
+    free(bounds);
+    return hres;
 }

static HRESULT interp_step(exec_ctx_t *ctx)

[Jason Millard (＠jsm174)]

@jacek - updated. Is this what you are looking for?

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/vbscript/interp.c:
>  array = V_ISBYREF(v) ? *V_ARRAYREF(v) : V_ARRAY(v);
>
>  hres = array_bounds_from_stack(ctx, dim_cnt, &bounds);
> - if(FAILED(hres))
> -
>    return hres;

Yes, it's better now, but this part was better without changes. When array_bounds_from_stack fails, bounds in not allocated, so the quick exit is fine.