[PATCH v2 0/1] MR1604: vbscript: Make redim fail on fixed-size arrays.

[Nikolay Sivov (＠nsivov)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53964
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>

--
  v2: vbscript: Make redim fail on fixed-size arrays.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1604

[PATCH v2 1/1] vbscript: Make redim fail on fixed-size arrays.

[Nikolay Sivov]

From: Nikolay Sivov <nsivov(a)codeweavers.com>

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53964
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
---
 dlls/vbscript/interp.c       | 23 +++++++++++-------
 dlls/vbscript/tests/lang.vbs | 46 +++++++++++++++++++++++++++++++-----
 dlls/vbscript/tests/run.c    | 33 ++++++++++++++++++++++++--
 3 files changed, 85 insertions(+), 17 deletions(-)

diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c
index 2d33982839e..b3be3bea6cf 100644
--- a/dlls/vbscript/interp.c
+++ b/dlls/vbscript/interp.c
@@ -1246,6 +1246,7 @@ static HRESULT interp_dim(exec_ctx_t *ctx)
         *array_ref = SafeArrayCreate(VT_VARIANT, array_desc->dim_cnt, array_desc->bounds);
         if(!*array_ref)
             return E_OUTOFMEMORY;
+        (*array_ref)->fFeatures |= (FADF_FIXEDSIZE | FADF_STATIC);
     }
 
     V_VT(v) = VT_ARRAY|VT_BYREF|VT_VARIANT;
@@ -1302,6 +1303,18 @@ static HRESULT interp_redim(exec_ctx_t *ctx)
         return E_FAIL;
     }
 
+    v = ref.u.v;
+
+    if(V_VT(v) == (VT_VARIANT|VT_BYREF)) {
+        v = V_VARIANTREF(v);
+    }
+
+    if(V_ISARRAY(v)) {
+        SAFEARRAY *sa = V_ISBYREF(v) ? *V_ARRAYREF(v) : V_ARRAY(v);
+        if(sa->fFeatures & FADF_FIXEDSIZE)
+            return MAKE_VBSERROR(VBSE_ARRAY_LOCKED);
+    }
+
     hres = array_bounds_from_stack(ctx, dim_cnt, &bounds);
     if(FAILED(hres))
         return hres;
@@ -1311,14 +1324,6 @@ static HRESULT interp_redim(exec_ctx_t *ctx)
     if(!array)
         return E_OUTOFMEMORY;
 
-    /* FIXME: We should check if we're not modifying an existing static array here */
-
-    v = ref.u.v;
-
-    if(V_VT(v) == (VT_VARIANT|VT_BYREF)) {
-        v = V_VARIANTREF(v);
-    }
-
     VariantClear(v);
     V_VT(v) = VT_ARRAY|VT_VARIANT;
     V_ARRAY(v) = array;
@@ -1361,7 +1366,7 @@ static HRESULT interp_redim_preserve(exec_ctx_t *ctx)
         return E_FAIL;
     }
 
-    array = V_ARRAY(v);
+    array = V_ISBYREF(v) ? *V_ARRAYREF(v) : V_ARRAY(v);
 
     hres = array_bounds_from_stack(ctx, dim_cnt, &bounds);
     if(FAILED(hres))
diff --git a/dlls/vbscript/tests/lang.vbs b/dlls/vbscript/tests/lang.vbs
index ddfaf20a19d..38cfa39a8e7 100644
--- a/dlls/vbscript/tests/lang.vbs
+++ b/dlls/vbscript/tests/lang.vbs
@@ -1531,12 +1531,46 @@ e = err.number
 on error goto 0
 ok e = 9, "e = " & e ' VBSE_OUT_OF_BOUNDS, can only change rightmost dimension
 
-dim staticarray(4)
-on error resume next
-redim staticarray(3)
-e = err.number
-on error goto 0
-todo_wine_ok e = 10, "e = " & e
+sub TestReDimFixed
+    on error resume next
+
+    dim staticarray(4)
+    err.clear
+    redim staticarray(3)
+    call ok(err.number = 10, "err.number = " & err.number)
+    call ok(isArrayFixed(staticarray), "Expected fixed size array")
+
+    err.clear
+    redim staticarray("abc")
+    call ok(err.number = 10, "err.number = " & err.number)
+
+    dim staticarray2(4)
+    err.clear
+    redim preserve staticarray2(5)
+    call ok(err.number = 10, "err.number = " & err.number)
+    call ok(isArrayFixed(staticarray2), "Expected fixed size array")
+
+    err.clear
+    redim preserve staticarray2("abc")
+    call todo_wine_ok(err.number = 5, "err.number = " & err.number)
+end sub
+Call TestRedimFixed
+
+sub TestRedimInputArg
+    on error resume next
+
+    dim x
+
+    x = Array(1)
+    err.clear
+    redim x("abc")
+    call ok(err.number = 13, "err.number = " & err.number)
+
+    err.clear
+    redim preserve x("abc")
+    call todo_wine_ok(err.number = 5, "err.number = " & err.number)
+end sub
+Call TestRedimInputArg
 
 sub TestReDimList
     dim x, y
diff --git a/dlls/vbscript/tests/run.c b/dlls/vbscript/tests/run.c
index 31ac03ab8d7..022d714dea5 100644
--- a/dlls/vbscript/tests/run.c
+++ b/dlls/vbscript/tests/run.c
@@ -146,6 +146,7 @@ DEFINE_EXPECT(OnLeaveScript);
 #define DISPID_GLOBAL_PROPARGSET      1025
 #define DISPID_GLOBAL_UNKOBJ          1026
 #define DISPID_GLOBAL_THROWEXCEPTION  1027
+#define DISPID_GLOBAL_ISARRAYFIXED    1028
 
 #define DISPID_TESTOBJ_PROPGET      2000
 #define DISPID_TESTOBJ_PROPPUT      2001
@@ -568,7 +569,6 @@ static void test_safearray(SAFEARRAY *safearray, unsigned indims)
     if(!exdims)
         exdims = 1;
     ok(safearray->cDims == exdims, "safearray->cDims = %d, expected %d\n", safearray->cDims, exdims);
-    todo_wine
     ok(safearray->fFeatures == (FADF_VARIANT|FADF_HAVEVARTYPE|FADF_FIXEDSIZE|FADF_STATIC),
        "safearray->fFeatures = %x\n", safearray->fFeatures);
     ok(safearray->cbElements == sizeof(VARIANT), "safearray->cbElements = %lx\n", safearray->cbElements);
@@ -1163,7 +1163,8 @@ static HRESULT WINAPI Global_GetDispID(IDispatchEx *iface, BSTR bstrName, DWORD
         { L"testOptionalArg", DISPID_GLOBAL_TESTOPTIONALARG },
         { L"testErrorObject", DISPID_GLOBAL_TESTERROROBJECT },
         { L"throwWithDesc",   DISPID_GLOBAL_THROWWITHDESC },
-        { L"unkObj",          DISPID_GLOBAL_UNKOBJ }
+        { L"unkObj",          DISPID_GLOBAL_UNKOBJ },
+        { L"isArrayFixed",    DISPID_GLOBAL_ISARRAYFIXED },
     };
 
     test_grfdex(grfdex, fdexNameCaseInsensitive);
@@ -1737,6 +1738,34 @@ static HRESULT WINAPI Global_InvokeEx(IDispatchEx *iface, DISPID id, LCID lcid,
         V_VT(pvarRes) = VT_UNKNOWN;
         V_UNKNOWN(pvarRes) = &unkObj;
         return S_OK;
+
+    case DISPID_GLOBAL_ISARRAYFIXED:
+    {
+        BOOL is_fixed = FALSE;
+        VARIANT *v;
+
+        ok(pdp != NULL, "pdp == NULL\n");
+        ok(pdp->rgvarg != NULL, "rgvarg == NULL\n");
+        ok(!pdp->rgdispidNamedArgs, "rgdispidNamedArgs != NULL\n");
+        ok(pdp->cArgs == 1, "cArgs = %d\n", pdp->cArgs);
+        ok(!pdp->cNamedArgs, "cNamedArgs = %d\n", pdp->cNamedArgs);
+        ok(pvarRes != NULL, "pvarRes == NULL\n");
+        ok(V_VT(pvarRes) == VT_EMPTY, "V_VT(pvarRes) = %d\n", V_VT(pvarRes));
+        ok(pei != NULL, "pei == NULL\n");
+
+        ok(V_VT(pdp->rgvarg) == (VT_VARIANT|VT_BYREF), "Unexpected argument type %d.\n", V_VT(pdp->rgvarg));
+        v = V_VARIANTREF(pdp->rgvarg);
+        ok(V_VT(v) == (VT_VARIANT|VT_ARRAY|VT_BYREF), "Unexpected argument type %d.\n", V_VT(v));
+        if (V_ISARRAY(v))
+        {
+            SAFEARRAY *sa = V_ISBYREF(v) ? *V_ARRAYREF(v) : V_ARRAY(v);
+            is_fixed = (sa->fFeatures & (FADF_FIXEDSIZE | FADF_STATIC)) == (FADF_FIXEDSIZE | FADF_STATIC);
+        }
+
+        V_VT(pvarRes) = VT_BOOL;
+        V_BOOL(pvarRes) = is_fixed ? VARIANT_TRUE : VARIANT_FALSE;
+        return S_OK;
+    }
     }
 
     ok(0, "unexpected call %ld\n", id);
-- 
GitLab

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

[PATCH v2 1/1] vbscript: Make redim fail on fixed-size arrays.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=127000

Your paranoid android.


=== w7u_2qxl (32 bit report) ===

vbscript:
run.c:1207: Test failed: lang.vbs: L"err.number = 13"
run.c:1207: Test failed: lang.vbs: L"err.number = 13"

=== w7u_adm (32 bit report) ===

vbscript:
run.c:1207: Test failed: lang.vbs: L"err.number = 13"
run.c:1207: Test failed: lang.vbs: L"err.number = 13"

=== w7u_el (32 bit report) ===

vbscript:
run.c:1207: Test failed: lang.vbs: L"err.number = 13"
run.c:1207: Test failed: lang.vbs: L"err.number = 13"

=== w1064v1507 (32 bit report) ===

vbscript:
run.c:1207: Test failed: lang.vbs: L"err.number = 13"
run.c:1207: Test failed: lang.vbs: L"err.number = 13"

=== w1064v1507 (64 bit report) ===

vbscript:
run.c:1207: Test failed: lang.vbs: L"err.number = 13"
run.c:1207: Test failed: lang.vbs: L"err.number = 13"

[Nikolay Sivov (＠nsivov)]

On Wed Nov 30 10:36:48 2022 +0000, Jacek Caban wrote:
> I think this should be checked earlier, before
> `array_bounds_from_stack()` call. This would avoid creating the new
> array and my testing confirms that's what Windows does (if I try to use
> "abc" as redim argument, VBSE_ARRAY_LOCKED is still returned on Windows
> rather than `to_int()` error).
> With a couple more tests, apparently "redim preserve" and "redim" are different in that regard. It's trivial to fix, I added todo_wine's for now, because it's unrelated to the original fix.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1604#note_18020