[PATCH 0/1] MR2131: vbscript: Fix memory leak in Split().

[Jason Millard (＠jsm174)]

While running in XCode's profiler, I noticed a memory leak in `Global_Split`.

When the strings are being copied into the SafeArray, the `BSTR` is not freed after `VariantCopyInd`.

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

[PATCH 1/1] vbscript: Fix memory leak in Split().

[Jason Millard]

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

---
 dlls/vbscript/global.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/dlls/vbscript/global.c b/dlls/vbscript/global.c
index 8c2e5b85982..605fd6a4cb0 100644
--- a/dlls/vbscript/global.c
+++ b/dlls/vbscript/global.c
@@ -2700,6 +2700,7 @@ static HRESULT Global_Split(BuiltinDisp *This, VARIANT *args, unsigned args_cnt,
         V_BSTR(&var) = str;
 
         hres = VariantCopyInd(data+i, &var);
+        SysFreeString(str);
         if(FAILED(hres)) {
             SafeArrayUnaccessData(sa);
             goto error;
-- 
GitLab

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

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/vbscript/global.c:
> V_BSTR(&var) = str;
>
> hres = VariantCopyInd(data+i, &var);
> +        SysFreeString(str);

This is fine for the leak, but it would be even better to avoid extra string copy. We don't really need `VariantCopyInd` in this case, we could just set data[i] variant directly and get rid of var variable.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2131#note_23411

[Jason Millard (＠jsm174)]

On Wed Feb  8 12:12:04 2023 +0000, Jacek Caban wrote:
> This is fine for the leak, but it would be even better to avoid extra
> string copy. We don't really need `VariantCopyInd` in this case, we
> could just set data[i] variant directly and get rid of var variable.
> Something like this?

```
    for (i = 0; i < count; i++) {
        V_VT(&data[i]) = VT_BSTR;
        V_BSTR(&data[i]) = SysAllocStringLen(string + start, indices[i] - start);
        
        if (!V_BSTR(&data[i])) {
            hres = E_OUTOFMEMORY;
       
            SafeArrayUnaccessData(sa);
            goto error;
        }
        start = indices[i]+delimiterlen;
    }
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2131#note_23434

[Jacek Caban (＠jacek)]

On Wed Feb  8 14:21:38 2023 +0000, Jason Millard wrote:
> Something like this?
> ```
>     for (i = 0; i < count; i++) {
>         V_VT(&data[i]) = VT_BSTR;
>         V_BSTR(&data[i]) = SysAllocStringLen(string + start, indices[i]
> - start);
>
> if (!V_BSTR(&data[i])) {
>             hres = E_OUTOFMEMORY;
>
> SafeArrayUnaccessData(sa);
>             goto error;
>         }
>         start = indices[i]+delimiterlen;
>     }
> ```
> Yes, but you could just use `break;` for error handling to avoid duplicating `SafeArrayUnaccessData` call.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2131#note_23437