WiX Util CA: Confusion around User rollback script open/create 'errors', and another possible use after free on CaScript 'handles'

[bevanweiss]

As I've been looking into this Create Group functionality, which I thought would just be a copy-paste of the Create User stuff, I've run into a few confusing situations. In particular when my Group already exists, and I'm trying to write rollback details, it keeps cancelling the install because it's trying to close this Script. But the script doesn't exist (WcaCaScriptOption reported file not found)
https://github.com/wixtoolset/wix/blob/96b546f04bb804ef661c676bc131ee050f07ac0d/src/ext/Util/ca/scaexec.cpp#L873-L879

    // Try to open the rollback script
    hr = WcaCaScriptOpen(WCA_ACTION_INSTALL, WCA_CASCRIPT_ROLLBACK, FALSE, pwzScriptKey, &hRollbackScript);
//                                                                                                                                pointer to struct  ^^^^^^^^^^^^^ 
    if (INVALID_HANDLE_VALUE != hRollbackScript)
//        ^^^^^^^^^^^^^^^^^ not a valid comparison... 
//      perhaps this should be (hRollbackScript && INVALID_HANDLE_VALUE != hRollbackScript->hScriptFile) instead?
    {
        WcaCaScriptClose(hRollbackScript, WCA_CASCRIPT_CLOSE_PRESERVE);
    }

and then when I went looking into the WcaCaScriptOpen and WcaCaScriptCreate functions, I've found a couple of use after free situations.

In [WcaCaScriptCreate], and in WcaCaScriptOpen(https://github.com/wixtoolset/wix/blob/96b546f04bb804ef661c676bc131ee050f07ac0d/src/libs/wcautil/WixToolset.WcaUtil/wcascript.cpp#L40-L83)

extern "C" HRESULT WIXAPI WcaCaScript{Create,Open}(
...
    __out WCA_CASCRIPT_HANDLE* phScript
    )
   (*phScript)->pwzScriptPath = pwzScriptPath;
//                        ^^^^^^^^^^^ we assign the LPWSTR to structure leaving the function... so we shouldn't release the memory
...
LExit:
...
    ReleaseStr(pwzScriptPath);
//  ^^^^^^^^^^^^^^^^^... but then we do
    return hr;

The first problem (the INVALID_HANDLE_VALUE compare) is probably relatively trivial to fix. Either:

• pointer check before checking the actual handle value; or
• just looking to see if the HRESULT was S_OK. If so, the rollback should be closed; or
• do away with the whole open, and just Create with non-append (overwrite).. I'm unsure what having a rollback file already existing when doing the CreateUser would mean..
  Any thoughts on this? If there's an opinion in any direction, I could put this into my Create Group logic first.