C++ Running SHELLEXECUTEINFO and appending feature name

Question

I dont know c++ and im having issues trying to append a string name to lpFile of SHELLEXECUTEINFO. I get a semi-colon delimited string from Installshield and then split it up and loop through it. I am trying to then append each one of the 'features' that were split from the string and send it to dism.exe through shell execute.

Its failing at swprintf_s(buf, _T("Dism.exe /Online /Enable-Feature /FeatureName:%s"), sizeof(buf), wc); If i comment that outh then it will at least fire off the dll and throw a dism.exe error, so i know the call is working correctly.

template<typename Out>
void split(const std::string &s, char delim, Out result) {
    std::stringstream ss;
    ss.str(s);
    std::string item;
    while (std::getline(ss, item, delim)) {
        *(result++) = item;
    }
}


HRESULT __stdcall SplitString(IDispatch *pAction) {
    // Create a pointer to the IsSuiteExtension COM interface
    CComQIPtr<ISuiteExtension2> spSuiteExtension2 = pAction;

    // Turn on notifications for both the progress bar(epfProgressValid) and the ui message(epfMessageValid).
    EnumProgressFlags pf = EnumProgressFlags(EnumProgressFlags::epfMessageValid | EnumProgressFlags::epfProgressValid);

    BSTR bstrFeatureList(_T("ENABLE_FEATURES"));  // Property name to get. This should be a semi-colon delimeted list of features to enable for windows.
    BSTR FeatureList = ::SysAllocStringLen(NULL, 2 * 38); // Where to store the property value
    HRESULT hRet = spSuiteExtension2->get_Property(bstrFeatureList, &FeatureList); // Get the property value and store it in the 'FeatureList' variable

    CW2A pszConverted(FeatureList);

    using namespace std;
    string strConverted(pszConverted);
    vector<string> tokens;
    split(strConverted, ';', back_inserter(tokens));
    int numTokens = tokens.size();
    for (int i = 0; i < numTokens; i++)
    {
        string t = tokens.at(i);
        TCHAR buf[1024];

        SHELLEXECUTEINFO ShExecInfo = { 0 };
        ShExecInfo.cbSize = sizeof(SHELLEXECUTEINFO);
        ShExecInfo.fMask = SEE_MASK_NOCLOSEPROCESS;
        ShExecInfo.hwnd = NULL;
        ShExecInfo.lpVerb = NULL;
        // Convert ANSI to Unicode
        ATL::CA2W wc(tokens.at(i).c_str());
        swprintf_s(buf, _T("Dism.exe /Online /Enable-Feature /FeatureName:%s"), sizeof(buf), wc);  // HAVING ISSUES HERE. NEED TO APPEND tokens.at(i) AT %S
        ShExecInfo.lpFile = buf;
        ShExecInfo.lpParameters = _T("");
        ShExecInfo.lpDirectory = _T("C:\\Windows\\SysNative");
        ShExecInfo.nShow = SW_SHOW;
        ShExecInfo.hInstApp = NULL;
        ShellExecuteEx(&ShExecInfo);
        WaitForSingleObject(ShExecInfo.hProcess, INFINITE);

    }

    //MessageBoxA(NULL, strConverted.c_str(), "testx", MB_OK);

    return ERROR_SUCCESS;
}

What do you mean by "failing"? Are you getting a compiler error? What is it? You've got an odd mix of "ANSI" and wide character string stuff going on here. — Adrian McCarthy, Jun 01 '17 at 20:52
I assume you're using Visual Studio. Right-click on the project name, choose Properties, then choose General under Configuration Properties and tell us if the Character Set field is set to "Use Unicode Character Set" or "Use Multi-Byte Character Set" (or something else). — Adrian McCarthy, Jun 01 '17 at 20:55
In Installshield it fails, silently. Throws error 0x80070643. There are NO compiler errors in VS — user616, Jun 01 '17 at 21:00
Note that unless you control your working directory and path, it's a potential security risk to launch an exe without an explicit path. It would be better to retrieve the system folder path and append dism.exe to that, resulting in a path like `C:\Windows\System32\Dism.exe`. (This applies as well to the CreateProcess case in Remy's answer.) — Michael Urman, Jun 02 '17 at 12:35
This looks like it's an action running in an InstallShield Suite installer; is there something preventing you from using the provided support for [Enabling Windows Roles and Features](http://helpnet.installshield.com/installshield22helplib/helplibrary/SteWindowsFeat.htm)? It's not limited to the features list in the dropdown. — Michael Urman, Jun 02 '17 at 12:39
@MichaelUrman Yes there are Windows 10 Creators Update bugs with DISM and it errors out the installer, causing a rollback. This is an error on microsofts side but we cant wait for them to issue a patch. — user616, Jun 02 '17 at 14:50

Remy Lebeau · Accepted Answer · 2017-06-01T21:52:55.570

Since your code is using TCHAR strings, you should be using _stprintf_s() instead of swprintf_s(). And as Adrian McCarthy said, you are passing its parameters in the wrong order, so your code shouldn't even compile to begin with.

But more importantly, there is no need to involve ANSI or TCHAR at all. You have Unicode input, and the STL and ShellExecuteEx() both support Unicode, so just keep everything in Unicode only.

You are also leaking the BSTR that you allocate for FeatureList, as well as the process HANDLE that ShellExecuteEx() returns.

Try something more like this instead:

template <typename Out>
void split(const std::wstring &s, wchar_t delim, Out result) {
    std::wistringstream iss(s);
    std::wstring item;
    while (std::getline(iss, item, delim)) {
        *(result++) = item;
    }
}

HRESULT __stdcall SplitString(IDispatch *pAction) {
    // Create a pointer to the IsSuiteExtension COM interface
    CComQIPtr<ISuiteExtension2> spSuiteExtension2 = pAction;

    // Turn on notifications for both the progress bar(epfProgressValid) and the ui message(epfMessageValid).
    EnumProgressFlags pf = EnumProgressFlags(EnumProgressFlags::epfMessageValid | EnumProgressFlags::epfProgressValid);

    CComBSTR FeatureList;
    HRESULT hRet = spSuiteExtension2->get_Property(CComBSTR(L"ENABLE_FEATURES"), &FeatureList); // Get the property value and store it in the 'FeatureList' variable

    std::vector<std::wstring> tokens;
    split(static_cast<BSTR>(FeatureList), L';', std::back_inserter(tokens));

    int numTokens = tokens.size();
    for (int i = 0; i < numTokens; i++)
    {
        std::wstring params = L"/Online /Enable-Feature /FeatureName:" + tokens.at(i);

        SHELLEXECUTEINFOW ShExecInfo = { 0 };
        ShExecInfo.cbSize = sizeof(ShExecInfo);
        ShExecInfo.fMask = SEE_MASK_NOCLOSEPROCESS;
        ShExecInfo.lpFile = L"Dism.exe";
        ShExecInfo.lpParameters = params.c_str();
        ShExecInfo.lpDirectory = L"C:\\Windows\\SysNative";
        ShExecInfo.nShow = SW_SHOW;

        if (ShellExecuteExW(&ShExecInfo))
        {
            WaitForSingleObject(ShExecInfo.hProcess, INFINITE);
            CloseHandle(ShExecInfo.hProcess);
        }
    }

    //MessageBoxW(NULL, FeatureList, L"testx", MB_OK);

    return ERROR_SUCCESS;
}

That being said, when launching an EXE, you should be using CreateProcess() directly instead of ShellExecuteEx():

for (int i = 0; i < numTokens; i++)
{
    std::wstring cmd = L"Dism.exe /Online /Enable-Feature /FeatureName:" + tokens.at(i);

    STARTUPINFO si = {0};
    PROCESS_INFORMATION pi = {0};

    si.cb = sizeof(si);
    si.dwFlags = STARTF_USESHOWWINDOW;
    si.wShowWindow = SW_SHOW;

    if (CreateProcessW(NULL, &cmd[0], NULL, NULL, FALSE, 0, NULL, L"C:\\Windows\\SysNative", &si, &pi))
    {
        CloseHandle(pi.hThread);
        WaitForSingleObject(pi.hProcess, INFINITE);
        CloseHandle(pi.hProcess);
    }
}

The mix of wide and TCHAR is concerning, and this would certainly be a better approach overall, possibly avoiding other bugs. — Adrian McCarthy, Jun 01 '17 at 21:21
If i replace just the first section of code you supplied, still using shellexecute, When compiling i get 2 errors: `'void split>>>(const std::wstring &,wchar_t,Out)' : cannot convert argument 1 from 'ATL::CComBSTR' to 'const std::wstring &' ` and `no instance of function template "split" matches the argument list argument types are: (ATL::CComBSTR, wchar_t, std::back_insert_iterator>>) ` — user616, Jun 01 '17 at 21:37
@user616: `CComBSTR` has an implicit `BSTR` conversion operator, `BSTR` is just an alias for `wchar_t*`, and `std::wstring` has a constructor that takes a `wchar_t*` as input. So I would expect the compiler to be smart enough to invoke the conversion operator automatically when passing a `CComBSTR` to a `std::wstring`. If not, just invoke the operator manually. I have updated my answer to show this. — Remy Lebeau, Jun 01 '17 at 21:52
@RemyLebeau Your edit fixed the issue. If i add in the second change you mentioned, using CreateProcess, the process just blows through immediately instead of waiting for the process to successfully complete. — user616, Jun 01 '17 at 22:09
@user616: `CreateProcess()` returns a `HANDLE` to the new process. That *should* be the same `HANDLE` that `ShellExecuteEx()` returns if it starts a new process. Is `CreateProcess()` succeeding or failing? If it is succeeding, but `WaitForSingleObject()` returns immediately, that means `Dism.exe` is exiting immediately. Perhaps the input is bad? Perhaps `Dism` is not the actual process that does the real work, but instead starts another process to do the work? You need to do some debugging to find out. — Remy Lebeau, Jun 01 '17 at 22:24
@user616: If `Dism` is starting a secondary process, you can create a [Job object](https://msdn.microsoft.com/en-us/library/windows/desktop/ms684161.aspx) to keep track of any child processes, and then wait on that job instead of the `HANDLE` returned by `CreateProcess()`. See [How do I wait until all processes in a job have exited?](https://blogs.msdn.microsoft.com/oldnewthing/20130405-00/?p=4743) for more details. — Remy Lebeau, Jun 01 '17 at 22:24

Adrian McCarthy · Answer 2 · 2017-06-01T23:14:35.043

You seem to have switched up the order of the parameters in the swprintf_s call. The buffer size should come immediately after the buffer and before the format string.

From MSDN:

int swprintf_s(  
   wchar_t *buffer,  
   size_t sizeOfBuffer,  
   const wchar_t *format,  
   ...  
);

From your code:

swprintf_s(buf, _T("Dism.exe /Online /Enable-Feature /FeatureName:%s"), sizeof(buf), wc);

I'm not sure how you got that to compile at all. It should be this instead:

swprintf_s(buf, sizeof(buf), _T("Dism.exe /Online /Enable-Feature /FeatureName:%s"), wc);

UPDATE: Here's why it compiled with the parameters in the wrong order:

In addition to the swprintf_s described in the MSDN documentation, there's a templated version with a signature like this:

template <std::size_t N>
int swprintf_s(wchar_t (&Buffer)[N], const wchar_t * format, ...);

Since the buf argument is indeed a fixed-length array (as opposed to a pointer to a buffer), this signature version is chosen (with N deduced from the length of buf), and thus the sizeof(buf) argument is treated as the string that corresponds to the %s and the actual string argument (wc) is ignored.

You're either ignoring or suppressing the compiler warnings. Sadly, the (very good) diagnostics emitted by VC++ in this case aren't treated as errors:

warning C4477: 'swprintf_s' : format string '%s' requires an argument of type 'wchar_t *', but variadic argument 1 has type 'unsigned int'
warning C4474: 'swprintf_s' : too many arguments passed for format string
note: placeholders and their parameters expect 1 variadic arguments, but 2 were provided

I wasnt getting any compilation errors but now that i made that change, Installshield does execute the dll correctly. I just get an error that i guess im not calling the dism tool correctly. — user616, Jun 01 '17 at 21:34

C++ Running SHELLEXECUTEINFO and appending feature name

2 Answers2