Salvage tests and fix a leak found in another review: see https://gitlab.winehq.org/wine/wine/-/merge_requests/897#note_9175 points 1 and 2. 3. is why !897 was reverted.
This does not contain the (incorrect) changes to VariantCopyInd, but instead fixes the leak identified in VariantClear (VT_RECORD needs to free pvRecord), and the wrong allocator being used in VariantCopy (it should have been IMalloc, rather than `HeapAlloc`, as shown by the fact IMallocSpy observes these allocations).
This also cleans up some dubious behavior by the test IRecordInfoImpl, which was modifying a VARIANT that it did not own or receive as an argument in the middle of VariantClear. This was likely undefined behavior, and in any case concealed the heap leak.
From: Kevin Puetz PuetzKevinA@JohnDeere.com
VariantClear should use IMalloc::Free() to clean up VT_RECORD. The test did not catch the leak becuse pvRecord was 0xdeadbeef, rather than an actual allocation. This would have crashed due to the bogus pointer, except that a pointer to the source VARIANT::BRECORD was smuggled into IRecordInfoImpl and used to overwrite the bogus pointer with NULL during RecordClear. VariantClear apparently does reload pvRecord between calling RecordClear and calling Free, but this seems brittle.
Use IMallocSpy to verify (and then filter out) the expected call freeing pvRecord, instead of sneakily making it a no-op Free(NULL). --- dlls/oleaut32/tests/vartest.c | 148 +++++++++++++++++++++++++++++++++- 1 file changed, 146 insertions(+), 2 deletions(-)
diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index cdbb836b041..2ae8b444233 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -299,7 +299,6 @@ static HRESULT WINAPI RecordInfo_RecordClear(IRecordInfo *iface, void *data) { IRecordInfoImpl* This = impl_from_IRecordInfo(iface); This->recordclear++; - This->rec->pvRecord = NULL; return S_OK; }
@@ -765,6 +764,131 @@ static const IUnknownVtbl test_VariantClear_vtbl = {
static test_VariantClearImpl test_myVariantClearImpl = {{&test_VariantClear_vtbl}, 1, 0};
+typedef struct IMallocSpyImpl +{ + IMallocSpy IMallocSpy_iface; + void *validptr; +} IMallocSpyImpl; + +static inline IMallocSpyImpl *impl_from_IMallocSpy(IMallocSpy *iface) +{ + return CONTAINING_RECORD(iface, IMallocSpyImpl, IMallocSpy_iface); +} + +static HRESULT WINAPI IMallocSpyImpl_QueryInterface(IMallocSpy *iface, REFIID riid, void **obj) +{ + if (IsEqualIID(riid, &IID_IMallocSpy) || IsEqualIID(riid, &IID_IUnknown)) + { + *obj = iface; + IMallocSpy_AddRef(iface); + return S_OK; + } + + return E_NOINTERFACE; +} + +static ULONG WINAPI IMallocSpyImpl_AddRef(IMallocSpy *iface) +{ + return 2; +} + +static ULONG WINAPI IMallocSpyImpl_Release(IMallocSpy *iface) +{ + return 1; +} + +static SIZE_T WINAPI IMallocSpyImpl_PreAlloc(IMallocSpy *iface, SIZE_T cb) +{ + return cb; +} + +static void* WINAPI IMallocSpyImpl_PostAlloc(IMallocSpy *iface, void *ptr) +{ + return ptr; +} + +static void* WINAPI IMallocSpyImpl_PreFree(IMallocSpy *iface, void *ptr, BOOL spyed) +{ + IMallocSpyImpl* This = impl_from_IMallocSpy(iface); + if(This->validptr && ptr == This->validptr) + { + This->validptr = NULL; + /* allow (but swallow) a prearranged pretend-it's-valid ptr */ + return NULL; + } + ok(spyed, "freed %p not allocated by this spy\n",ptr); + return ptr; +} + +static void WINAPI IMallocSpyImpl_PostFree(IMallocSpy *iface, BOOL spyed) +{ +} + +static SIZE_T WINAPI IMallocSpyImpl_PreRealloc(IMallocSpy *iface, void *ptr, SIZE_T cb, void **newptr, BOOL spyed) +{ + ok(0, "unexpected call\n"); + return 0; +} + +static void* WINAPI IMallocSpyImpl_PostRealloc(IMallocSpy *iface, void *ptr, BOOL spyed) +{ + ok(0, "unexpected call\n"); + return NULL; +} + +static void* WINAPI IMallocSpyImpl_PreGetSize(IMallocSpy *iface, void *ptr, BOOL spyed) +{ + ok(0, "unexpected call\n"); + return ptr; +} + +static SIZE_T WINAPI IMallocSpyImpl_PostGetSize(IMallocSpy *iface, SIZE_T actual, BOOL spyed) +{ + ok(0, "unexpected call\n"); + return actual; +} + +static void* WINAPI IMallocSpyImpl_PreDidAlloc(IMallocSpy *iface, void *ptr, BOOL spyed) +{ + ok(0, "unexpected call\n"); + return ptr; +} + +static int WINAPI IMallocSpyImpl_PostDidAlloc(IMallocSpy *iface, void *ptr, BOOL spyed, int actual) +{ + ok(0, "unexpected call\n"); + return actual; +} + +static void WINAPI IMallocSpyImpl_PreHeapMinimize(IMallocSpy *iface) +{ + ok(0, "unexpected call\n"); +} + +static void WINAPI IMallocSpyImpl_PostHeapMinimize(IMallocSpy *iface) +{ + ok(0, "unexpected call\n"); +} + +static const IMallocSpyVtbl IMallocSpyImpl_vtbl = +{ + IMallocSpyImpl_QueryInterface, + IMallocSpyImpl_AddRef, + IMallocSpyImpl_Release, + IMallocSpyImpl_PreAlloc, + IMallocSpyImpl_PostAlloc, + IMallocSpyImpl_PreFree, + IMallocSpyImpl_PostFree, + IMallocSpyImpl_PreRealloc, + IMallocSpyImpl_PostRealloc, + IMallocSpyImpl_PreGetSize, + IMallocSpyImpl_PostGetSize, + IMallocSpyImpl_PreDidAlloc, + IMallocSpyImpl_PostDidAlloc, + IMallocSpyImpl_PreHeapMinimize, + IMallocSpyImpl_PostHeapMinimize +}; + static void test_VariantClear(void) { struct __tagBRECORD *rec; @@ -775,6 +899,7 @@ static void test_VariantClear(void) size_t i; LONG i4; IUnknown *punk; + IMallocSpyImpl malloc_spy = { {&IMallocSpyImpl_vtbl} };
/* Crashes: Native does not test input for NULL, so neither does Wine */ if (0) @@ -889,21 +1014,29 @@ static void test_VariantClear(void) /* Check that nothing got called */ ok(test_myVariantClearImpl.events == 0, "Unexpected call. events %08lx\n", test_myVariantClearImpl.events);
+ hres = CoRegisterMallocSpy(&malloc_spy.IMallocSpy_iface); + ok(hres == S_OK, "ret 0x%08lx\n", hres); + /* RECORD */ recinfo = get_test_recordinfo(); V_VT(&v) = VT_RECORD; rec = &V_UNION(&v, brecVal); rec->pRecInfo = &recinfo->IRecordInfo_iface; rec->pvRecord = (void*)0xdeadbeef; + malloc_spy.validptr = (void*)0xdeadbeef; recinfo->recordclear = 0; recinfo->ref = 2; recinfo->rec = rec; hres = VariantClear(&v); ok(hres == S_OK, "ret %08lx\n", hres); - ok(rec->pvRecord == NULL, "got %p\n", rec->pvRecord); + ok(V_RECORD(&v) == (void*)0xdeadbeef, "got %p\n",V_RECORD(&v)); + todo_wine ok(!malloc_spy.validptr, "%p not freed\n",malloc_spy.validptr); ok(recinfo->recordclear == 1, "got %d\n", recinfo->recordclear); ok(recinfo->ref == 1, "got %ld\n", recinfo->ref); IRecordInfo_Release(&recinfo->IRecordInfo_iface); + + hres = CoRevokeMallocSpy(); + ok(hres == S_OK, "ret 0x%08lx\n", hres); }
static void test_VariantCopy(void) @@ -914,6 +1047,7 @@ static void test_VariantCopy(void) VARTYPE vt; size_t i; HRESULT hres, hExpected; + IMallocSpyImpl malloc_spy = { {&IMallocSpyImpl_vtbl} };
/* Establish that the failure/other cases are dealt with. Individual tests * for each type should verify that data is copied correctly, references @@ -1026,6 +1160,9 @@ static void test_VariantCopy(void) VariantClear(&vDst); }
+ hres = CoRegisterMallocSpy(&malloc_spy.IMallocSpy_iface); + ok(hres == S_OK, "ret 0x%08lx\n", hres); + /* copy RECORD */ recinfo = get_test_recordinfo();
@@ -1050,8 +1187,15 @@ static void test_VariantCopy(void) ok(recinfo->getsize == 1, "got %d\n", recinfo->recordclear); ok(recinfo->recordcopy == 1, "got %d\n", recinfo->recordclear);
+ malloc_spy.validptr = NULL; VariantClear(&vDst); + + malloc_spy.validptr = (void*)0xdeadbeef; VariantClear(&vSrc); + todo_wine ok(!malloc_spy.validptr, "%p not freed\n",malloc_spy.validptr); + + hres = CoRevokeMallocSpy(); + ok(hres == S_OK, "ret 0x%08lx\n", hres); }
/* Determine if a vt is valid for VariantCopyInd() */
From: Kevin Puetz PuetzKevinA@JohnDeere.com
V_BYREF owns nothing; it does not Release pRecInfo or Destroy/Free pvRecord --- dlls/oleaut32/tests/vartest.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index 2ae8b444233..4a8f5d844a9 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -1035,6 +1035,21 @@ static void test_VariantClear(void) ok(recinfo->ref == 1, "got %ld\n", recinfo->ref); IRecordInfo_Release(&recinfo->IRecordInfo_iface);
+ /* RECORD BYREF */ + recinfo = get_test_recordinfo(); + V_VT(&v) = VT_RECORD | VT_BYREF; + V_RECORDINFO(&v) = &recinfo->IRecordInfo_iface; + V_RECORD(&v) = (void*)0xdeadbeef; + malloc_spy.validptr = NULL; /* BYREF will not free */ + recinfo->recordclear = 0; + recinfo->ref = 1; /* BYREF does not own a refcount */ + hres = VariantClear(&v); + ok(hres == S_OK, "ret %08lx\n", hres); + ok(V_RECORD(&v) == (void*)0xdeadbeef, "got %p\n",V_RECORD(&v)); + ok(recinfo->recordclear == 0, "got %d\n", recinfo->recordclear); + ok(recinfo->ref == 1, "got %ld\n", recinfo->ref); + IRecordInfo_Release(&recinfo->IRecordInfo_iface); + hres = CoRevokeMallocSpy(); ok(hres == S_OK, "ret 0x%08lx\n", hres); }
From: Kevin Puetz PuetzKevinA@JohnDeere.com
In many cases (in particular when VariantCopy has been used), multiple VARIANT structs which contain the same type of record will share the same instance of IRecordInfo, just adding refcounts.
RecordClear should not be aware of where the data it's clearing came from, and certainly should not be modifying someone else's VARIANT::brecVal. This seems to have been intended as a way to make tests more sensitive to use-after-free, by overwriting the source pointer with NULL after clearing the pointed-to record.
To preserve that sensitivity (if it was indeed the goal), replace the hardcoded "0xdeadbeef is valid" check with a "validsrc" address that get_test_recordinfo initially agrees to pretend points to a valid record, but will stop accepting after a call to RecordClear. --- dlls/oleaut32/tests/vartest.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index 4a8f5d844a9..25a603fa644 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -249,7 +249,7 @@ typedef struct IRecordInfoImpl unsigned int recordclear; unsigned int getsize; unsigned int recordcopy; - struct __tagBRECORD *rec; + void *validsrc; } IRecordInfoImpl;
static inline IRecordInfoImpl *impl_from_IRecordInfo(IRecordInfo *iface) @@ -299,6 +299,8 @@ static HRESULT WINAPI RecordInfo_RecordClear(IRecordInfo *iface, void *data) { IRecordInfoImpl* This = impl_from_IRecordInfo(iface); This->recordclear++; + if(data == This->validsrc) + This->validsrc = NULL; /* not valid anymore, now that it's been cleared */ return S_OK; }
@@ -306,7 +308,7 @@ static HRESULT WINAPI RecordInfo_RecordCopy(IRecordInfo *iface, void *src, void { IRecordInfoImpl* This = impl_from_IRecordInfo(iface); This->recordcopy++; - ok(src == (void*)0xdeadbeef, "wrong src pointer %p\n", src); + ok(This->validsrc && (src == This->validsrc), "wrong src pointer %p\n", src); return S_OK; }
@@ -891,7 +893,6 @@ static const IMallocSpyVtbl IMallocSpyImpl_vtbl =
static void test_VariantClear(void) { - struct __tagBRECORD *rec; IRecordInfoImpl *recinfo; HRESULT hres; VARIANTARG v; @@ -1020,13 +1021,11 @@ static void test_VariantClear(void) /* RECORD */ recinfo = get_test_recordinfo(); V_VT(&v) = VT_RECORD; - rec = &V_UNION(&v, brecVal); - rec->pRecInfo = &recinfo->IRecordInfo_iface; - rec->pvRecord = (void*)0xdeadbeef; + V_RECORDINFO(&v) = &recinfo->IRecordInfo_iface; + V_RECORD(&v) = (void*)0xdeadbeef; malloc_spy.validptr = (void*)0xdeadbeef; recinfo->recordclear = 0; recinfo->ref = 2; - recinfo->rec = rec; hres = VariantClear(&v); ok(hres == S_OK, "ret %08lx\n", hres); ok(V_RECORD(&v) == (void*)0xdeadbeef, "got %p\n",V_RECORD(&v)); @@ -1056,7 +1055,6 @@ static void test_VariantClear(void)
static void test_VariantCopy(void) { - struct __tagBRECORD *rec; IRecordInfoImpl *recinfo; VARIANTARG vSrc, vDst; VARTYPE vt; @@ -1185,20 +1183,18 @@ static void test_VariantCopy(void) V_VT(&vDst) = VT_EMPTY;
V_VT(&vSrc) = VT_RECORD; - rec = &V_UNION(&vSrc, brecVal); - rec->pRecInfo = &recinfo->IRecordInfo_iface; - rec->pvRecord = (void*)0xdeadbeef; + V_RECORDINFO(&vSrc) = &recinfo->IRecordInfo_iface; + V_RECORD(&vSrc) = (void*)0xdeadbeef;
recinfo->recordclear = 0; recinfo->recordcopy = 0; recinfo->getsize = 0; - recinfo->rec = rec; + recinfo->validsrc = (void*)0xdeadbeef; hres = VariantCopy(&vDst, &vSrc); ok(hres == S_OK, "ret %08lx\n", hres);
- rec = &V_UNION(&vDst, brecVal); - ok(rec->pvRecord != (void*)0xdeadbeef && rec->pvRecord != NULL, "got %p\n", rec->pvRecord); - ok(rec->pRecInfo == &recinfo->IRecordInfo_iface, "got %p\n", rec->pRecInfo); + ok(V_RECORD(&vDst) != (void*)0xdeadbeef && V_RECORD(&vDst) != NULL, "got %p\n", V_RECORD(&vDst)); + ok(V_RECORDINFO(&vDst) == &recinfo->IRecordInfo_iface, "got %p\n", V_RECORDINFO(&vDst)); ok(recinfo->getsize == 1, "got %d\n", recinfo->recordclear); ok(recinfo->recordcopy == 1, "got %d\n", recinfo->recordclear);
From: Kevin Puetz PuetzKevinA@JohnDeere.com
--- dlls/oleaut32/tests/vartest.c | 46 ++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index 25a603fa644..3cf37290487 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -328,7 +328,7 @@ static HRESULT WINAPI RecordInfo_GetSize(IRecordInfo *iface, ULONG* size) { IRecordInfoImpl* This = impl_from_IRecordInfo(iface); This->getsize++; - *size = 0; + *size = 1; return S_OK; }
@@ -770,6 +770,8 @@ typedef struct IMallocSpyImpl { IMallocSpy IMallocSpy_iface; void *validptr; + void *lastalloc; + SIZE_T cbAlloc; } IMallocSpyImpl;
static inline IMallocSpyImpl *impl_from_IMallocSpy(IMallocSpy *iface) @@ -801,11 +803,15 @@ static ULONG WINAPI IMallocSpyImpl_Release(IMallocSpy *iface)
static SIZE_T WINAPI IMallocSpyImpl_PreAlloc(IMallocSpy *iface, SIZE_T cb) { + IMallocSpyImpl* This = impl_from_IMallocSpy(iface); + ok(This->cbAlloc && cb == This->cbAlloc, "unexpected allocation size=%Iu\n",cb); return cb; }
static void* WINAPI IMallocSpyImpl_PostAlloc(IMallocSpy *iface, void *ptr) { + IMallocSpyImpl* This = impl_from_IMallocSpy(iface); + This->lastalloc = ptr; return ptr; }
@@ -818,6 +824,8 @@ static void* WINAPI IMallocSpyImpl_PreFree(IMallocSpy *iface, void *ptr, BOOL sp /* allow (but swallow) a prearranged pretend-it's-valid ptr */ return NULL; } + if(ptr == This->lastalloc) + This->lastalloc = NULL; ok(spyed, "freed %p not allocated by this spy\n",ptr); return ptr; } @@ -1190,16 +1198,19 @@ static void test_VariantCopy(void) recinfo->recordcopy = 0; recinfo->getsize = 0; recinfo->validsrc = (void*)0xdeadbeef; + malloc_spy.cbAlloc = 1; hres = VariantCopy(&vDst, &vSrc); ok(hres == S_OK, "ret %08lx\n", hres);
ok(V_RECORD(&vDst) != (void*)0xdeadbeef && V_RECORD(&vDst) != NULL, "got %p\n", V_RECORD(&vDst)); ok(V_RECORDINFO(&vDst) == &recinfo->IRecordInfo_iface, "got %p\n", V_RECORDINFO(&vDst)); + todo_wine ok(malloc_spy.lastalloc == V_RECORD(&vDst), "%p allocation not seen by IMallocSpy\n",V_RECORD(&vDst)); ok(recinfo->getsize == 1, "got %d\n", recinfo->recordclear); ok(recinfo->recordcopy == 1, "got %d\n", recinfo->recordclear);
malloc_spy.validptr = NULL; VariantClear(&vDst); + ok(!malloc_spy.lastalloc, "%p not freed\n",malloc_spy.lastalloc);
malloc_spy.validptr = (void*)0xdeadbeef; VariantClear(&vSrc); @@ -1230,6 +1241,8 @@ static void test_VariantCopyInd(void) size_t i; BYTE buffer[64]; HRESULT hres, hExpected; + IRecordInfoImpl *recinfo; + IMallocSpyImpl malloc_spy = {{&IMallocSpyImpl_vtbl}, 0, NULL };
memset(buffer, 0, sizeof(buffer));
@@ -1420,6 +1433,37 @@ static void test_VariantCopyInd(void) hres = VariantCopyInd(&vDst, &vSrc); ok(hres == E_INVALIDARG, "CopyInd(ref->ref): expected E_INVALIDARG, got 0x%08lx\n", hres); + + hres = CoRegisterMallocSpy(&malloc_spy.IMallocSpy_iface); + ok(hres == S_OK, "ret 0x%08lx\n", hres); + + /* Copy RECORD BYREF */ + recinfo = get_test_recordinfo(); + V_VT(&vSrc) = VT_RECORD|VT_BYREF; + V_RECORDINFO(&vSrc) = &recinfo->IRecordInfo_iface; + V_RECORD(&vSrc) = (void*)0xdeadbeef; + + VariantInit(&vDst); + recinfo->validsrc = (void*)0xdeadbeef; + malloc_spy.cbAlloc = 1; + hres = VariantCopyInd(&vDst, &vSrc); + ok(hres == S_OK, "VariantCopyInd failed: 0x%08lx\n", hres); + ok(V_VT(&vDst) == VT_RECORD, "got vt = %d|0x%X\n", V_VT(&vDst) & VT_TYPEMASK, V_VT(&vDst) & ~VT_TYPEMASK); + ok(V_RECORDINFO(&vDst) == &recinfo->IRecordInfo_iface, "got %p\n", V_RECORDINFO(&vDst)); + ok(recinfo->recordclear == 0,"got %d\n", recinfo->recordclear); + ok(V_RECORD(&vDst) != (void*)0xdeadbeef, "expected a newly-allocated deep copy\n"); + todo_wine ok(malloc_spy.lastalloc == V_RECORD(&vDst), "%p allocation not seen by IMallocSpy\n",V_RECORD(&vDst)); + ok(recinfo->getsize == 1,"got %d\n", recinfo->getsize); + ok(recinfo->recordcopy == 1,"got %d\n", recinfo->recordcopy); + ok(recinfo->ref == 2,"got %ld\n", recinfo->ref); + + VariantClear(&vDst); + ok(recinfo->ref == 1,"got %ld\n", recinfo->ref); + ok(recinfo->recordclear == 1,"got %d\n", recinfo->recordclear); + ok(!malloc_spy.lastalloc, "%p not freed\n",malloc_spy.lastalloc); + + hres = CoRevokeMallocSpy(); + ok(hres == S_OK, "ret 0x%08lx\n", hres); }
static HRESULT (WINAPI *pVarParseNumFromStr)(const OLECHAR*,LCID,ULONG,NUMPARSE*,BYTE*);
From: Kevin Puetz PuetzKevinA@JohnDeere.com
VariantCopy was using the wrong allocator; VariantClear simply leaked --- dlls/oleaut32/tests/vartest.c | 8 ++++---- dlls/oleaut32/variant.c | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index 3cf37290487..efbc93ba756 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -1037,7 +1037,7 @@ static void test_VariantClear(void) hres = VariantClear(&v); ok(hres == S_OK, "ret %08lx\n", hres); ok(V_RECORD(&v) == (void*)0xdeadbeef, "got %p\n",V_RECORD(&v)); - todo_wine ok(!malloc_spy.validptr, "%p not freed\n",malloc_spy.validptr); + ok(!malloc_spy.validptr, "%p not freed\n",malloc_spy.validptr); ok(recinfo->recordclear == 1, "got %d\n", recinfo->recordclear); ok(recinfo->ref == 1, "got %ld\n", recinfo->ref); IRecordInfo_Release(&recinfo->IRecordInfo_iface); @@ -1204,7 +1204,7 @@ static void test_VariantCopy(void)
ok(V_RECORD(&vDst) != (void*)0xdeadbeef && V_RECORD(&vDst) != NULL, "got %p\n", V_RECORD(&vDst)); ok(V_RECORDINFO(&vDst) == &recinfo->IRecordInfo_iface, "got %p\n", V_RECORDINFO(&vDst)); - todo_wine ok(malloc_spy.lastalloc == V_RECORD(&vDst), "%p allocation not seen by IMallocSpy\n",V_RECORD(&vDst)); + ok(malloc_spy.lastalloc == V_RECORD(&vDst), "%p allocation not seen by IMallocSpy\n",V_RECORD(&vDst)); ok(recinfo->getsize == 1, "got %d\n", recinfo->recordclear); ok(recinfo->recordcopy == 1, "got %d\n", recinfo->recordclear);
@@ -1214,7 +1214,7 @@ static void test_VariantCopy(void)
malloc_spy.validptr = (void*)0xdeadbeef; VariantClear(&vSrc); - todo_wine ok(!malloc_spy.validptr, "%p not freed\n",malloc_spy.validptr); + ok(!malloc_spy.validptr, "%p not freed\n",malloc_spy.validptr);
hres = CoRevokeMallocSpy(); ok(hres == S_OK, "ret 0x%08lx\n", hres); @@ -1452,7 +1452,7 @@ static void test_VariantCopyInd(void) ok(V_RECORDINFO(&vDst) == &recinfo->IRecordInfo_iface, "got %p\n", V_RECORDINFO(&vDst)); ok(recinfo->recordclear == 0,"got %d\n", recinfo->recordclear); ok(V_RECORD(&vDst) != (void*)0xdeadbeef, "expected a newly-allocated deep copy\n"); - todo_wine ok(malloc_spy.lastalloc == V_RECORD(&vDst), "%p allocation not seen by IMallocSpy\n",V_RECORD(&vDst)); + ok(malloc_spy.lastalloc == V_RECORD(&vDst), "%p allocation not seen by IMallocSpy\n",V_RECORD(&vDst)); ok(recinfo->getsize == 1,"got %d\n", recinfo->getsize); ok(recinfo->recordcopy == 1,"got %d\n", recinfo->recordcopy); ok(recinfo->ref == 2,"got %ld\n", recinfo->ref); diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c index 885082cc659..a0ac7d0254e 100644 --- a/dlls/oleaut32/variant.c +++ b/dlls/oleaut32/variant.c @@ -654,6 +654,7 @@ HRESULT WINAPI DECLSPEC_HOTPATCH VariantClear(VARIANTARG* pVarg) IRecordInfo_RecordClear(pBr->pRecInfo, pBr->pvRecord); IRecordInfo_Release(pBr->pRecInfo); } + CoTaskMemFree(pBr->pvRecord); } else if (V_VT(pVarg) == VT_DISPATCH || V_VT(pVarg) == VT_UNKNOWN) @@ -689,7 +690,7 @@ static HRESULT VARIANT_CopyIRecordInfo(VARIANT *dest, const VARIANT *src) /* This could look cleaner if only RecordCreate() was used, but native doesn't use it. Memory should be allocated in a same way as RecordCreate() does, so RecordDestroy() could free it later. */ - dest_rec->pvRecord = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size); + dest_rec->pvRecord = CoTaskMemAlloc(size); if (!dest_rec->pvRecord) return E_OUTOFMEMORY;
dest_rec->pRecInfo = src_rec->pRecInfo;
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=124909
Your paranoid android.
=== debian11 (build log) ===
Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24694. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24694. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24694.
Certainly worth a second, or third opinion, but to me this test looks like something to keep out of a tree.
On Wed Oct 12 18:06:20 2022 +0000, Nikolay Sivov wrote:
Certainly worth a second, or third opinion, but to me this test looks like something to keep out of a tree.
I assume by "keep out of a tree" you're saying you'd want to merge the fix but not all of the tests? Which part do you want separated: the IMallocSpy, the cleanups to IRecordInfoImpl::RecordClear, something else?
Seeing the allocations via IMallocSpy was the best way I could think of to actually demonstrate VariantCopy was using CoTaskMemAlloc/IMalloc, but I'm fine if we don't think there needs to be a permanent conformance test to justify that choice. It is kind of the obvious choice for code in oleaut32, albeit someone before apparently overlooked it and used a HeapAlloc instead (which doesn't match windows, and matters if anyone is mixing IRecordInfo::RecordCreate and IRecordInfo::RecordDestroy with VariantCopy/VariantClear.
I don't think the test code as written before was really even correct - there's no particular reason to be confident VariantClear reloads pvRecord from the original VARIANT after calling RecordClear; it seemingly does, but per strict-aliasing it would certainly have been allowed to just reuse the value already in a register or some such too. Just up to how the compiler optimized things (or, in practice, seemingly didn't). And it seems clearly wrong that VariantClear(&varDest) actually modified varSrc too (via the RecordClear modifying varSrc.brecVal via IRecordInfoImpl::rec - https://gitlab.winehq.org/wine/wine/-/merge_requests/1035/diffs?commit_id=02...). There was no test actually verifying varSrc (why would there be?), and it really didn't concern anything done wrong by oleauto, but it seems like a bad behavior to have in the test fixture.
And if we remove that hack from IRecordInfoImpl (or someday when MS's implementation gets a little more compiler optimization), the existing tests start crashing on windows (as seen in https://gitlab.winehq.org/wine/wine/-/merge_requests/897#note_9110). And after fixing the mising free/memleak in wine's VariantClear, then it crashes on wine too.
If you'd rather, we could eliminate 0xdeadbeef/validsrc/validptr etc and instead implement `RecordInfo_RecordCreate`, so that the pvRecord values used are all actually allocated. That's a bigger change to the actual coverage of the tests, though.
On Wed Oct 12 18:07:13 2022 +0000, Kevin Puetz wrote:
I assume by "keep out of a tree" you're saying you'd want to merge the fix but not all of the tests? Which part do you want separated: the IMallocSpy, the cleanups to IRecordInfoImpl::RecordClear, something else? Seeing the allocations via IMallocSpy was the best way I could think of to actually demonstrate VariantCopy was using CoTaskMemAlloc/IMalloc, but I'm fine if we don't think there needs to be a permanent conformance test to justify that choice. It is kind of the obvious choice for code in oleaut32, albeit someone before apparently overlooked it and used a HeapAlloc instead (which doesn't match windows, and matters if anyone is mixing IRecordInfo::RecordCreate and IRecordInfo::RecordDestroy with VariantCopy/VariantClear. I don't think the test code as written before was really even correct - there's no particular reason to be confident VariantClear reloads pvRecord from the original VARIANT after calling RecordClear; it seemingly does, but per strict-aliasing it would certainly have been allowed to just reuse the value already in a register or some such too. Just up to how the compiler optimized things (or, in practice, seemingly didn't). And it seems clearly wrong that VariantClear(&varDest) actually modified varSrc too (via the RecordClear modifying varSrc.brecVal via IRecordInfoImpl::rec - https://gitlab.winehq.org/wine/wine/-/merge_requests/1035/diffs?commit_id=02...). There was no test actually verifying varSrc (why would there be?), and it really didn't concern anything done wrong by oleauto, but it seems like a bad behavior to have in the test fixture. And if we remove that hack from IRecordInfoImpl (or someday when MS's implementation gets a little more compiler optimization), the existing tests start crashing on windows (as seen in https://gitlab.winehq.org/wine/wine/-/merge_requests/897#note_9110). And after fixing the mising free/memleak in wine's VariantClear, then it crashes on wine too. If you'd rather, we could eliminate 0xdeadbeef/validsrc/validptr etc and instead implement `RecordInfo_RecordCreate`, so that the pvRecord values used are all actually allocated. That's a bigger change to the actual coverage of the tests, though.
Yes, I meant IMallocSpy part, usual concern is to look too much into implementation details. In this case it's really easy to do, and tracing API is of course public. But now that you know how to allocate/free correctly, you don't really need spying part. It's just my opinion, I'd wait for someone else to comment on this too.
On Thu Oct 13 14:48:54 2022 +0000, Nikolay Sivov wrote:
Yes, I meant IMallocSpy part, usual concern is to look too much into implementation details. In this case it's really easy to do, and tracing API is of course public. But now that you know how to allocate/free correctly, you don't really need spying part. It's just my opinion, I'd wait for someone else to comment on this too.
Yep, the spy is there for two reasons. One is to "show my work" with a conformance test that demonstrates this is indeed supposed to be IMalloc. I.e. showing this implementation choice is externally-visible from documented interfaces. But it's probably enough that the MR shows reviewers that proof, it doesn't have to make it into the tree.
I could also have done this part with `IMalloc::DidAlloc`, though it's not a very good test since wine's implementation of that is semi-stub; HeapValidate will accept pointes that didn't come through IMalloc too. So I think the test work work as intended on windows, and it would pass on the fixed wine, but it would also false-positive pass on the still-broken wine. So that would be an option to document the intention (less invasively), though it's not actually a very sensitive regression test.
The other purpose is that also let me filter out the Free(0xdeadbeef) that could be pretty symmetrical to RecordInfo_RecordCopy. So if we drop it, we'd also have to switch the tests to actually-valid allocations (making them a little less sensitive to use-after-free or whether the record was supposed to be deep-copied or shallow-copied).
But I care more about the actual fix, I'm fine with whatever level of test coverage the you guys actually want to keep :-)
-
Kevin Puetz -
Kevin Puetz (＠puetzk)
-
Marvin -
Nikolay Sivov (＠nsivov)