[PATCH 0/1] MR2131: vbscript: Fix memory leak in Split().
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
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) 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
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
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
participants (3)
-
Jacek Caban (@jacek) -
Jason Millard -
Jason Millard (@jsm174)