bitcrazed / paws

Tool to run a specified executable and then pause before terminating

Hi there 👋

paws's Issues

Couple of issues

Nice little exercise but it has various issues.

---

Paws/Paws/Paws.cpp

Line 13 in c9877d2

enum Flags { Quiet, Verbose };

Unused code.

---

Paws/Paws/Paws.cpp

Line 16 in c9877d2

void DisplayUsage(bool fVTEnabled);

Unused parameter.

---

Paws/Paws/Paws.cpp

Line 21 in c9877d2

// Globals:

No need for global variables. Consider to put them into a class and pass a constant reference to the functions where coloring is used.

---

Paws/Paws/Paws.cpp

Lines 47 to 53 in c9877d2

	// Extract the command-line sans the name of this executable:
	wstring strFullCommandLine{ (GetCommandLine()) };
	wstring strThisExe{ argv[0] };
	strThisExe.append(L" ");
	size_t nStart{ strThisExe.length() + 2 };
	size_t nLength{ strFullCommandLine.length() - strThisExe.length() - 1 };
	wstring strCommandLine{ strFullCommandLine.substr(nStart, nLength) };

Bug
You can't compare the length of argv[0] with the length of the first token in the command line. Surrounding quotes are removed in the argv elements. So, you will have different lengths if you call the program quoted or unquoted. Think about rolling your own parser to skip the first token. It's pretty straightforward. (See the next sub-issue.)

---

Paws/Paws/Paws.cpp

Line 61 in c9877d2

const_cast<LPWSTR>(strCommandLine.c_str()),

Bug
This causes undefined behavior. CreateProcessW will alter the string as the documentation states.
As suggested before, parse the command line in a custom function and find the pointer to the second token. Use the C-string rather than an STL string since the string returned by GetCommandLineW is mutable as CreateProcessW requires it. E.g.:

LPWSTR GetCmdLnSkipFirstToken()
{
    static LPWSTR cmdLn{};
    bool isInQuotes{};
    bool isInToken{};
    DWORD tokensCnt{};

    if (cmdLn == nullptr)
        cmdLn = GetCommandLineW();

    for (LPWCH cursor{ cmdLn }; *cursor != L'\0'; ++cursor)
    {
        switch (*cursor)
        {
        case L' ': case L'\t': // Token separators, unless in a quoted substring
            if (isInQuotes == false)
                isInToken = false;
            break;

        case L'\"':
            isInQuotes = !isInQuotes;
        default: // This includes quotation marks
            if (isInToken == false)
            {
                ++tokensCnt;
                if (tokensCnt > 1) // That is, the cursor points to the beginning of the second token
                    return cursor;

                isInToken = true;
            }
            break;
        }
    }

    return nullptr;
}

Further advantages:

• you don't need to copy the C-strings into STL string classes
• you can use main() rather than wmain() to eliminate compiler dependencies because you don't need argc and argv anymore. Just call your DisplayUsage if nullptr was returned by the above function

---

Paws/Paws/Paws.cpp

Lines 72 to 73 in c9877d2

	WaitForSingleObject(pi.hProcess, INFINITE);
	}

Thread handle and process handle have to be closed according to the specification. This is missing after WaitForSingleObject here.

---

Paws/Paws/Paws.cpp

Line 76 in c9877d2

wcout << szVTRed << L"Error: Paws failed to execute:" << GetErrorMessage() << szVTReset << endl;

It's an error message. Consider to write to the wcerr stream.

---

Paws/Paws/Paws.cpp

Line 82 in c9877d2

}

The main routine implicitely returns 0, always. What about catching the exit code of the executed command for a proper return value. Or perhaps use the last-error code if CreateProcessW failed.

---

Paws/Paws/Paws.cpp

Line 89 in c9877d2

while (0 == _kbhit());

This polling loop causes a noticeable increase of CPU usage. Consider to replace this line with
static_cast<void>(_getwch());

---

Paws/Paws/Paws.cpp

Line 115 in c9877d2

lpMsgBuf,

Bug
This doesn't work because you effectively pass a null pointer. The parameter type is quite misleading in case you specified FORMAT_MESSAGE_FROM_SYSTEM. Nevertheless you have to pass the address of the pointer variable here.
reinterpret_cast<LPWSTR>(&lpMsgBuf),

---

Paws/Paws/Paws.cpp

Line 118 in c9877d2

wstring msg{ lpMsgBuf };

Bug
Since you don't check if FormatMessageW was successful you may cause undefined behavior here if you pass a null pointer to the constructor of the string.

---

Paws/Paws/Paws.cpp

Lines 125 to 135 in c9877d2

	bool EnableVTProcessing()
	{
	DWORD consoleMode{};
	HRESULT hr{ E_UNEXPECTED };
	HANDLE hConsole = { GetStdHandle(STD_OUTPUT_HANDLE) };
	GetConsoleMode(hConsole, &consoleMode);
	hr = SetConsoleMode(hConsole, consoleMode | ENABLE_VIRTUAL_TERMINAL_PROCESSING)
	? S_OK
	: GetLastError();
	return S_OK == hr;
	}

This is cumbersome. Proposal:

bool EnableVTProcessing()
{
    DWORD consoleMode{};
    HANDLE hConsole = { GetStdHandle(STD_OUTPUT_HANDLE) };
    return (GetConsoleMode(hConsole, &consoleMode) != FALSE &&
        SetConsoleMode(hConsole, consoleMode | ENABLE_VIRTUAL_TERMINAL_PROCESSING) != FALSE);
}

---

A couple of ANSI string literals are used for the output to the wcout stream.

---

Best regards
Steffen

Function clear() of struct VTColors seems to be wrong

In function clear() of struct VTColors the string.empty() calls tests whether string is empty but does not clear the string, I think you want to call string.clear() instead.