[PATCH v6 0/1] MR2175: oleaut32: fix VarAbs function for BSTR with positive values

[Jason Millard (＠jsm174)]

While working in vbscript, I noticed abs on a `BSTR` value with a positive number would come back with an invalid value.

Given the following script:

```
Dim x
x = "30000"
Debug.Print "x" & "=" & abs(x)
```

Outputs:

```
Script.Print 'x=3.08221696253945E-314'
```

After debugging, `pVarIn` gets the R8 value but is never shallow copied into `pVarOut`.

I'm not sure how to write a test case in `oleaut32`, so I included a test case in `vbscript`.

--
  v6: oleaut32: fix VarAbs function for BSTR with positive values

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

[PATCH v6 1/1] oleaut32: fix VarAbs function for BSTR with positive values

[Jason Millard]

From: Jason Millard

---
 dlls/oleaut32/tests/vartest.c | 7 +++++++
 dlls/oleaut32/variant.c       | 3 ++-
 dlls/vbscript/tests/api.vbs   | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c
index cdbb836b041..331866c9272 100644
--- a/dlls/oleaut32/tests/vartest.c
+++ b/dlls/oleaut32/tests/vartest.c
@@ -3057,6 +3057,7 @@ static HRESULT (WINAPI *pVarAbs)(LPVARIANT,LPVARIANT);
 static void test_VarAbs(void)
 {
     static WCHAR szNum[] = {'-','1','.','1','\0' };
+    static WCHAR szNum2[] = {'3','0','0','0','0','\0' };
     char buff[8];
     HRESULT hres;
     VARIANT v, vDst, exp;
@@ -3143,6 +3144,12 @@ static void test_VarAbs(void)
     hres = pVarAbs(&v,&vDst);
     ok(hres == S_OK && V_VT(&vDst) == VT_R8 && V_R8(&vDst) == 1.1,
        "VarAbs: expected 0x0,%d,%g, got 0x%lX,%d,%g\n", VT_R8, 1.1, hres, V_VT(&vDst), V_R8(&vDst));
+    V_VT(&v) = VT_BSTR;
+    V_BSTR(&v) = (BSTR)szNum2;
+    memset(&vDst,0,sizeof(vDst));
+    hres = pVarAbs(&v,&vDst);
+    ok(hres == S_OK && V_VT(&vDst) == VT_R8 && V_R8(&vDst) == 30000.0,
+       "VarAbs: expected 0x0,%d,%g, got 0x%lX,%d,%g\n", VT_R8, 30000.0, hres, V_VT(&vDst), V_R8(&vDst));
 }

static HRESULT (WINAPI *pVarNot)(LPVARIANT,LPVARIANT);
diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c
index 885082cc659..f3242ea5dc6 100644
--- a/dlls/oleaut32/variant.c
+++ b/dlls/oleaut32/variant.c
@@ -4374,8 +4374,9 @@ HRESULT WINAPI VarAbs(LPVARIANT pVarIn, LPVARIANT pVarOut)
         hRet = VarR8FromStr(V_BSTR(pVarIn), LOCALE_USER_DEFAULT, 0, &V_R8(&varIn));
         if (FAILED(hRet))
             break;
-        V_VT(pVarOut) = VT_R8;
         pVarIn = &varIn;
+        *pVarOut = *pVarIn;
+        V_VT(pVarOut) = VT_R8;
         /* Fall through ... */
     case VT_DATE:
     ABS_CASE(R8,R8_MIN);
diff --git a/dlls/vbscript/tests/api.vbs b/dlls/vbscript/tests/api.vbs
index c0ac6c56cf5..842cce33f83 100644
--- a/dlls/vbscript/tests/api.vbs
+++ b/dlls/vbscript/tests/api.vbs
@@ -1672,6 +1672,7 @@ Call ok(Abs(True) = 1, "Abs(True) = " & Abs(True))
 Call ok(getVT(Abs(True)) = "VT_I2", "getVT(Abs(True)) = " & getVT(Abs(True)))
 Call ok(Abs(CByte(1)) = 1, "Abs(CByte(1)) = " & Abs(CByte(1)))
 Call ok(getVT(Abs(CByte(1))) = "VT_UI1", "getVT(Abs(CByte(1))) = " & getVT(Abs(CByte(1))))
+Call ok(Abs("30000") = 30000, "Abs(""30000"") = " & Abs("30000"))

Sub testAbsError(strings, error_num1, error_num2)
     on error resume next

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/oleaut32/tests/vartest.c:
>  hres = pVarAbs(&v,&vDst);
>  ok(hres == S_OK && V_VT(&vDst) == VT_R8 && V_R8(&vDst) == 1.1,
>     "VarAbs: expected 0x0,%d,%g, got 0x%lX,%d,%g\n", VT_R8, 1.1, hres, V_VT(&vDst), V_R8(&vDst));
> - V_VT(&v) = VT_BSTR;
> - V_BSTR(&v) = (BSTR)szNum2;

This is not exactly right, BSTRs should be allocated with SysAllocString (they have "hidden" length member). I guess you copied that from the existing test and it's not really wrong: this proves that being a real BSTR doesn't matter in this particular case. But given that it's not the intention of this test, I'd suggest to just use BSTR properly.

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/oleaut32/variant.c:
>      hRet = VarR8FromStr(V_BSTR(pVarIn), LOCALE_USER_DEFAULT, 0, &V_R8(&varIn));
>      if (FAILED(hRet))
>          break;
> -
>    V_VT(pVarOut) = VT_R8;
>    pVarIn = &varIn;
> -
>    *pVarOut = *pVarIn;
> -
>    V_VT(pVarOut) = VT_R8;

This will work, but while reviewing this I noticed that current handling of VT_R4 and VT_R8 is not exactly right, overflow checks don't really make sense. Fixing that allows to simplify VT_BSTR case a bit. Please take a look at https://gitlab.winehq.org/jacek/wine/-/commits/varabs/ and if it looks good to you, update MR with that branch (it also contains BSTR allocation fixup).