[PATCH v2 0/2] MR897: oleauto: dereference VT_RECORD|VT_BYREF in place
VariantCopyInd allows pvargDest == pvargSrc in order to dereference in place To avoid confusing the source values and a partially-written destination, wine's implementation makes a shallow copy and uses that as pSrc. However, the call to VARIANT_CopyIRecordInfo did not use this, leading to it copying from the zeroed-out memory it just allocated. -- v2: oleaut32: dereference VT_RECORD|VT_BYREF in place. oleaut32/tests: get_test_recordinfo shouldn't point into a specific VARIANT. https://gitlab.winehq.org/wine/wine/-/merge_requests/897
From: Kevin Puetz <PuetzKevinA(a)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 | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index cdbb836b041..342133d6403 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -246,10 +246,10 @@ typedef struct IRecordInfoImpl { IRecordInfo IRecordInfo_iface; LONG ref; + void *validsrc; unsigned int recordclear; unsigned int getsize; unsigned int recordcopy; - struct __tagBRECORD *rec; } IRecordInfoImpl; static inline IRecordInfoImpl *impl_from_IRecordInfo(IRecordInfo *iface) @@ -299,7 +299,8 @@ static HRESULT WINAPI RecordInfo_RecordClear(IRecordInfo *iface, void *data) { IRecordInfoImpl* This = impl_from_IRecordInfo(iface); This->recordclear++; - This->rec->pvRecord = NULL; + if(data == This->validsrc) + This->validsrc = NULL; /* not valid anymore, now that it's been cleared */ return S_OK; } @@ -307,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; } @@ -430,6 +431,7 @@ static IRecordInfoImpl *get_test_recordinfo(void) rec->recordclear = 0; rec->getsize = 0; rec->recordcopy = 0; + rec->validsrc = (void *)0xdeadbeef; return rec; } @@ -767,7 +769,6 @@ static test_VariantClearImpl test_myVariantClearImpl = {{&test_VariantClear_vtbl static void test_VariantClear(void) { - struct __tagBRECORD *rec; IRecordInfoImpl *recinfo; HRESULT hres; VARIANTARG v; @@ -892,15 +893,12 @@ 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) = recinfo->validsrc; 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(recinfo->recordclear == 1, "got %d\n", recinfo->recordclear); ok(recinfo->ref == 1, "got %ld\n", recinfo->ref); IRecordInfo_Release(&recinfo->IRecordInfo_iface); @@ -908,7 +906,6 @@ static void test_VariantClear(void) static void test_VariantCopy(void) { - struct __tagBRECORD *rec; IRecordInfoImpl *recinfo; VARIANTARG vSrc, vDst; VARTYPE vt; @@ -1033,20 +1030,17 @@ 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) = recinfo->validsrc; recinfo->recordclear = 0; recinfo->recordcopy = 0; recinfo->getsize = 0; - recinfo->rec = rec; 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) != recinfo->validsrc && 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/897
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124069 Your paranoid android. === w864 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064v1507 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064v1809 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_2qxl (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_adm (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_tsign (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_en_AE_u8 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_ar (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_ja (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_zh_CN (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374)
From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com> VariantCopyInd allows pvargDest == pvargSrc in order to dereference in place To avoid confusing the source values and a partially-written destination, wine's implementation makes a shallow copy and uses that as pSrc. However, the call to VARIANT_CopyIRecordInfo did not use this, leading to it copying from the zeroed-out memory it just allocated. --- dlls/oleaut32/tests/vartest.c | 60 +++++++++++++++++++++++++++++++++++ dlls/oleaut32/variant.c | 2 +- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index 342133d6403..782142a3ca2 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -902,6 +902,19 @@ static void test_VariantClear(void) ok(recinfo->recordclear == 1, "got %d\n", recinfo->recordclear); 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) = recinfo->validsrc; + hres = VariantClear(&v); + ok(hres == S_OK, "ret %08lx\n", hres); + ok(V_VT(&v) == VT_EMPTY, "got vt = %d",V_VT(&v)); + /* BYREF does not own the pointed-to V_RECORD/INFO, so no RecordClear and no Release */ + ok(recinfo->recordclear == 0, "got %d\n", recinfo->recordclear); + ok(recinfo->ref == 1, "got %ld\n", recinfo->ref); + IRecordInfo_Release(&recinfo->IRecordInfo_iface); } static void test_VariantCopy(void) @@ -1069,6 +1082,7 @@ static void test_VariantCopyInd(void) size_t i; BYTE buffer[64]; HRESULT hres, hExpected; + IRecordInfoImpl *recinfo; memset(buffer, 0, sizeof(buffer)); @@ -1259,6 +1273,52 @@ static void test_VariantCopyInd(void) hres = VariantCopyInd(&vDst, &vSrc); ok(hres == E_INVALIDARG, "CopyInd(ref->ref): expected E_INVALIDARG, got 0x%08lx\n", hres); + + /* source data for a deep-copy of VT_BYREF|VT_RECORD... */ + recinfo = get_test_recordinfo(); + V_VT(&vSrc) = VT_RECORD|VT_BYREF; + V_RECORDINFO(&vSrc) = &recinfo->IRecordInfo_iface; + V_RECORD(&vSrc) = recinfo->validsrc; + + /* into a separate vDst */ + VariantInit(&vDst); + 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) != recinfo->validsrc, "expected a newly-allocated deep copy\n"); + 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); + + recinfo->getsize = 0; + recinfo->recordcopy = 0; + recinfo->recordclear = 0; + + /* and also in-place */ + hres = VariantCopyInd(&vSrc, &vSrc); + ok(V_VT(&vSrc) == VT_RECORD, "got vt = %d|0x%X\n", V_VT(&vSrc) & VT_TYPEMASK, V_VT(&vSrc) & ~VT_TYPEMASK); + ok(V_RECORDINFO(&vSrc) == &recinfo->IRecordInfo_iface, "got %p\n", V_RECORDINFO(&vSrc)); + /* ++ref and no RecordClear, since the source BYREF does not own the V_RECORD/INFO + * which it pointed to, and thus did not free them when overwritten */ + ok(recinfo->ref == 2,"got %ld\n", recinfo->ref); + ok(recinfo->recordclear == 0,"got %d\n", recinfo->recordclear); + + ok(V_RECORD(&vDst) != recinfo->validsrc, "expected a newly-allocated deep copy\n"); + ok(recinfo->getsize == 1,"got %d\n", recinfo->getsize); + ok(recinfo->recordcopy == 1,"got %d\n", recinfo->recordcopy); + + /* but the no-longer-BYREF output owns and will free its copies */ + VariantClear(&vSrc); + ok(recinfo->ref == 1,"got %ld\n", recinfo->ref); + ok(recinfo->recordclear == 1,"got %d\n", recinfo->recordclear); + + IRecordInfo_Release(&recinfo->IRecordInfo_iface); } static HRESULT (WINAPI *pVarParseNumFromStr)(const OLECHAR*,LCID,ULONG,NUMPARSE*,BYTE*); diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c index 885082cc659..ec0c9544a61 100644 --- a/dlls/oleaut32/variant.c +++ b/dlls/oleaut32/variant.c @@ -879,7 +879,7 @@ HRESULT WINAPI VariantCopyInd(VARIANT* pvargDest, const VARIANTARG* pvargSrc) } else if (V_VT(pSrc) == (VT_RECORD|VT_BYREF)) { - hres = VARIANT_CopyIRecordInfo(pvargDest, pvargSrc); + hres = VARIANT_CopyIRecordInfo(pvargDest, pSrc); } else if (V_VT(pSrc) == (VT_DISPATCH|VT_BYREF) || V_VT(pSrc) == (VT_UNKNOWN|VT_BYREF)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/897
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124070 Your paranoid android. === w7u_2qxl (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 002A4018 === w7u_adm (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00AD4018 === w7u_el (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00B74150 === w8 (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00B5B148 === w8adm (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00AEB148 === w864 (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00A7DAE0 === w1064v1507 (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00305260 === w1064v1809 (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00D38580 === w1064 (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00D88358 === w1064_tsign (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00F08258 === w10pro64 (32 bit report) === oleaut32: vartest.c:311: Test failed: wrong src pointer 00BF8230 === w864 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064v1507 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064v1809 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_2qxl (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_adm (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_tsign (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_en_AE_u8 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_ar (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_ja (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_zh_CN (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374)
Yeah, I was just being lazy since I thought writing a test IRecordInfo seemed like a lot of fiddling and the bug was really just the wrong source pointer. But fair game calling me out on it :-) get_test_recordinfo was indeed most of what was needed, though it doesn't play nicely with being copied; it held a pointer back into brecVal of one speific VARIANT, and used this to make RecordClear also set pvRecord =NULL. Which doesn't work out well when things have been shallow-copied into temporaries and it's now manipulating the wrong VARIANT struct. So I fixed that first (13271f77), hopefully preserving the intent behind it. One could also just get rid of it entirely, since that's just not a thing a real IRecordInfo would do, but it did seem like a potentially valuable for tests to have that extra fragility, to catching a use-after-free more consistently should there be one. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/897#note_9106
On Fri Sep 23 05:56:29 2022 +0000, **** wrote:
Marvin replied on the mailing list: ``` 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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124069 Your paranoid android. === w864 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064v1507 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064v1809 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_2qxl (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_adm (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w1064_tsign (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_en_AE_u8 (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_ar (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_ja (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) === w10pro64_zh_CN (64 bit report) === Report validation errors: oleaut32:vartest crashed (c0000374) ``` good bot :-(
Ok, so there's several parts here: 1. the crash is happening because test_VariantClear constructed a VT_RECORD pointing to 0xdeadbeef. When windows tries to free the heap memory at 0xdeadbeef, this is invalid and crashes. The test previously got away with it because RecordInfo_RecordClear would set This->rec->pvRecord = NULL, and it appears the code did happen to reload that from the STRUCT in between calling RecordClear and HeapFree. I don't think this was actually robust (it would seem legal under strict aliasing) for the compiler to optimize it into a single load of the V_RECORD(&v) and then pass that pointer to both RecordClear and HeapFree, but apparently the MS code must not done so in practice. wine doesn't hit this becuse it seems to just have a memleak - it doesn't have a HeapFree in VariantClear to balance the HeapAlloc in VARIANT_CopyIRecordInfo, and apparently it should. I don't really know why only crashed on 64-bit runs - it seemingly did the same thing (free 0xdeadbeef) on 32-bit ones, it just didn't take down the process. Probably differences in how the heap diagnostics work and what they can catch. 2. If VariantClear is going to free the source pointer, it really needs to be something valid, not 0xdeadbeef. But which allocator should be used doesn't seem to be documented API. One can see in the wine source code that VariantCopy uses HeapAlloc, but a the backtrace seen of the mingw-gdb crash suggests's the MS implementation involved c++ operator delete somehow (?), so I don't think it's actually just GetProcessHeap(), even if that's maybe what the defaults end up. I wonder if it's actually supposed to be CoTaskMemAlloc? That would at least make sense for an oleaut32 function... 3. The fix I was trying to make is not actually valid anyway - windows apparentrly has the same actual not-a-bug-I-guess in VariantCopyInd, and passes the same newly-allocated pointer (not yet initialized) to IRecordInfo::RecordCopy as both pvExisting and pvNew. I went back to the original app where I had hid this bug on wine (but worked on windows), and I've figured out what else was different that led to the windows run not actually calling this in-place VariantCopyInd. Mea Culpa, and now you are thoroughly justified in telling me to come back when I had a conformance test :-( In fact, I actually don't find the statement "if pvargSrc == pvargDest, this function dereferences in place" in the microsoft documentation for VariantCopyInd: https://learn.microsoft.com/en-us/windows/win32/api/oleauto/nf-oleauto-varia... - it just says
This function frees any existing contents of pvarDest.
Only in wine's documentation is there a claim that this is supposed to work: https://source.winehq.org/WineAPI/VariantCopyInd.html
- if pvargSrc == pvargDest, this function dereferences in place. Otherwise, pvargDest is always cleared using VariantClear before pvargSrc is copied to it. If clearing pvargDest fails, so does this function.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/897#note_9175
participants (3)
-
Kevin Puetz -
Kevin Puetz (＠puetzk)
-
Marvin