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.