[wix-devs] Debugging WIX4/Burn Unit tests

[Hoover, Jacob]

    size_t cchPath = 0;
…
    hr = ::StringCchLengthW(sczCanonicalizedPath, STRSAFE_MAX_CCH, &cchPath);
    PathExitOnFailure(hr, "Failed to get length of canonicalized path.");


    if (CSTR_EQUAL != ::CompareStringW(LOCALE_NEUTRAL, NORM_IGNORECASE, sczCanonicalizedDirectory, (DWORD)cchDirectory, sczCanonicalizedPath, cchDirectory < cchPath ? (DWORD)cchDirectory: (DWORD)cchPath))
    {
        ExitFunction1(hr = S_FALSE);
    }

Avoids the AV, however I am unsure on the next line..

   hr = sczCanonicalizedPath[cchDirectory] ? S_OK : S_FALSE;


From: wix-devs <wix-devs-bounces at lists.wixtoolset.org> On Behalf Of Sean Hall via wix-devs
Sent: Tuesday, November 1, 2022 3:05 PM
To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
Cc: Sean Hall <r.sean.hall at gmail.com>
Subject: Re: [wix-devs] Debugging WIX4/Burn Unit tests

> we shouldn’t be passing the same cchDirectory, the second one should
indicate the length of the second string, right?

Not exactly. We're trying to see if the second string starts with the first
string so it should be the same length for both. I probably thought that
this was safe because if the second string is shorter than the first one
then it should stop comparing once it gets to the null at the end of the
second string.

On Tue, Nov 1, 2022 at 2:40 PM Hoover, Jacob <Jacob.Hoover at greenheck.com<mailto:Jacob.Hoover at greenheck.com>>
wrote:

> On to the next failing test…
>
>
>
> DUtil, IniUtil, x64 mode fails in IniParse for the “setf ~PlainValue
> Blah…” test.
>
>
>
> Looking at the code, it seems inside the for loop, we don’t reset the
> pointer for wzValueNameStart, and as such, it causes the use of a pointer
> that’s been free’d.
>
>
>
> Adding the NULL allows this to succeed.
>
>
>
> wzValueNameStart = NULL;
>
>
>
> if (pi->sczValuePrefix)
>
> {
>
> wzValuePrefix = wcsstr(pi->rgsczLines[i], pi->sczValuePrefix);
>
> if (wzValuePrefix != NULL)
>
> {
>
> wzValueNameStart = wzValuePrefix + dwValuePrefixLength;
>
> }
>
> }
>
> else
>
> {
>
> wzValueNameStart = pi->rgsczLines[i];
>
> }
>
>
>
>
>
>
>
> DUtil, Path2Utl, PathDirectoryContainsPath
>
>
>
> hr = PathDirectoryContainsPath(L"C:\\Directory", L"C:\\");
>
>
>
>
>
> if (CSTR_EQUAL != ::CompareStringW(LOCALE_NEUTRAL, NORM_IGNORECASE,
> sczCanonicalizedDirectory, (DWORD)cchDirectory, sczCanonicalizedPath,
> (DWORD)cchDirectory))
>
>
>
> we shouldn’t be passing the same cchDirectory, the second one should
> indicate the length of the second string, right? (
> https://learn.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-comparestringw<https://learn.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-comparestringw>
> )
>
>
>
>
>
> *From:* wix-devs <wix-devs-bounces at lists.wixtoolset.org<mailto:wix-devs-bounces at lists.wixtoolset.org>> *On Behalf Of *Sean
> Hall via wix-devs
> *Sent:* Monday, October 31, 2022 9:38 PM
> *To:* WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org<mailto:wix-devs at lists.wixtoolset.org>>
> *Cc:* Sean Hall <r.sean.hall at gmail.com<mailto:r.sean.hall at gmail.com>>
> *Subject:* Re: [wix-devs] Debugging WIX4/Burn Unit tests
>
>
>
> I think you're right. I think the test should zero out the memory first. CI
> only builds in Release mode.
>
> On Mon, Oct 31, 2022 at 9:21 PM Hoover, Jacob <Jacob.Hoover at greenheck.com<mailto:Jacob.Hoover at greenheck.com>>
> wrote:
>
> > I am beginning to think this test is invalid…
> >
> >
> >
> > void VariantBasicTest()
> >
> > {
> >
> > BURN_VARIANT expectedVariants[10];
> >
> > BURN_VARIANT actualVariants[10];
> >
> > for (DWORD i = 0; i < 10; i++)
> >
> > {
> >
> > BVariantUninitialize(expectedVariants + i);
> >
> > BVariantUninitialize(actualVariants + i);
> >
> > }
> >
> > …
> >
> >
> >
> > typedef struct _BURN_VARIANT
> >
> > {
> >
> > union
> >
> > {
> >
> > LONGLONG llValue;
> >
> > VERUTIL_VERSION* pValue;
> >
> > LPWSTR sczValue;
> >
> > };
> >
> > BURN_VARIANT_TYPE Type;
> >
> > } BURN_VARIANT;
> >
> >
> >
> >
> >
> > Since it’s an array of BURN_VARIANT, what ensures the memory will be zero
> > allocated?
> >
> >
> >
> > I ask because using GFlags, going heap paranoid, and it only chokes on
> > this test. (In theory I’ve changed nothing that should cause this.)
> >
> >
> >
> >
> >
> > In other words, should we add
> >
> >
> >
> > memset(expectedVariants, 0, sizeof(expectedVariants));
> >
> > memset(actualVariants, 0, sizeof(actualVariants));
> >
> >
> >
> > after we allocate them, or just not call
> >
> >
> >
> > for (DWORD i = 0; i < 10; i++)
> >
> > {
> >
> > BVariantUninitialize(expectedVariants + i);
> >
> > BVariantUninitialize(actualVariants + i);
> >
> > }
> >
> >
> >
> >
> >
> > Does CI/CD build and run both debug and release mode?
> >
> >
> >
> > *From:* wix-devs <wix-devs-bounces at lists.wixtoolset.org<mailto:wix-devs-bounces at lists.wixtoolset.org>> *On Behalf Of
> *Sean
> > Hall via wix-devs
> > *Sent:* Monday, October 31, 2022 4:33 PM
> > *To:* WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org<mailto:wix-devs at lists.wixtoolset.org>>
> > *Cc:* Sean Hall <r.sean.hall at gmail.com<mailto:r.sean.hall at gmail.com>>
> > *Subject:* Re: [wix-devs] Debugging WIX4/Burn Unit tests
> >
> >
> >
> > Yes. It builds and runs them in x86 and x64.
> >
> > On Mon, Oct 31, 2022 at 4:24 PM Hoover, Jacob via wix-devs <
> > wix-devs at lists.wixtoolset.org<mailto:wix-devs at lists.wixtoolset.org>> wrote:
> >
> > > Trying to test my changes, and I've run into an issue where the unit
> test
> > > host process is crashing. Note this only happens on x64, and it's
> > > triggering a breakpoint from
> > >
> > > [Fact]
> > > void VariantBasicTest()
> > > {
> > > BURN_VARIANT expectedVariants[10];
> > > BURN_VARIANT actualVariants[10];
> > > for (DWORD i = 0; i < 10; i++)
> > > {
> > > BVariantUninitialize(expectedVariants + i); // This is the
> > > line causing the issue...
> > > BVariantUninitialize(actualVariants + i);
> > > }
> > >
> > > BVariantUninitialize
> > >
> >
> ->StrSecureZeroFreeString->StrSecureZeroString->StrSize->MemSizeChecked->MemSize
> > >
> > > extern "C" SIZE_T DAPI MemSize(
> > > __in LPCVOID pv
> > > )
> > > {
> > > // AssertSz(vfMemInitialized, "MemInitialize() not called, this would
> > > normally crash");
> > > return ::HeapSize(::GetProcessHeap(), 0, pv);
> > > }
> > >
> > > HeapSize is stating that the Heap has been corrupted.
> > >
> > > Does our CI pipeline build and run the x64 tests?
> > >
> > > Thanks,
> > > Jacob
> > >
> > > ____________________________________________________________________
> > > WiX Toolset Developer Mailing List provided by FireGiant
> > > http://www.firegiant.com/<http://www.firegiant.com>
> > >
> > ____________________________________________________________________
> > WiX Toolset Developer Mailing List provided by FireGiant
> > http://www.firegiant.com/<http://www.firegiant.com/>
> >
> > NOTE: This email was received from an external source. Please use caution
> > when opening links or attachments in the message.
> >
> ____________________________________________________________________
> WiX Toolset Developer Mailing List provided by FireGiant
> http://www.firegiant.com/<http://www.firegiant.com/>
>
> NOTE: This email was received from an external source. Please use caution
> when opening links or attachments in the message.
>
____________________________________________________________________
WiX Toolset Developer Mailing List provided by FireGiant http://www.firegiant.com/<http://www.firegiant.com/>
NOTE: This email was received from an external source. Please use caution when opening links or attachments in the message.