A simple C WinAPI program for terminating processes via process image names - follow-up 4

Question

(See the previous version here.)

What's new

1. get_last_err_msg renamed to MyGetLastErrorMessage
2. The caller to MyGetLastErrorMessage is responsible for freeing the string
3. The error messages are set to NULL after freeing
4. Instead of char*, a WinAPI type LPCSTR is used

Code

#include <stdio.h>
#include <windows.h>
#include <shlwapi.h>
#include <TlHelp32.h>
#pragma comment(lib, "Shlwapi.lib")

static LPCSTR MyGetLastErrorMessage() {
    DWORD errorMessageId = GetLastError();

    if (errorMessageId == 0) {
        return "no errors";
    }

    LPCSTR messageBuffer = NULL;
    size_t size =
        FormatMessageA(
            FORMAT_MESSAGE_ALLOCATE_BUFFER | 
            FORMAT_MESSAGE_FROM_SYSTEM | 
            FORMAT_MESSAGE_IGNORE_INSERTS, 
            NULL, 
            errorMessageId, 
            MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), 
            (LPSTR)& messageBuffer,
            65535, // MSDN tells max length is 64K
            NULL);

    return messageBuffer;
}

int main(int argc, char* argv[]) {
    if (argc != 2) {
        char* bname = _strdup(argv[0]);
        PathStripPath(bname);
        fprintf(stderr, "%s PROCESS_NAME\n", bname);
        free(bname);
        return EXIT_FAILURE;
    }

    PROCESSENTRY32 entry;
    entry.dwSize = sizeof(PROCESSENTRY32);
    HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, NULL);

    if (snapshot == INVALID_HANDLE_VALUE) {
        LPCSTR szLastErrorMessage = MyGetLastErrorMessage();

        fprintf(
            stderr,
            "Error: could not get the process snapshot. "
            "Cause: %s\n", szLastErrorMessage);

        free((void*) szLastErrorMessage);
        szLastErrorMessage = NULL;
        return EXIT_FAILURE;
    }

    size_t totalProcesses = 0;
    size_t totalProcessesMatched = 0;
    size_t totalProcessesTerminated = 0;

    if (Process32First(snapshot, &entry)) {
        do {
            totalProcesses++;

            if (strcmp(entry.szExeFile, argv[1]) != 0) {
                continue;
            }

            totalProcessesMatched++;

            HANDLE hProcess = OpenProcess(PROCESS_TERMINATE, 
                                          FALSE, 
                                          entry.th32ProcessID);

            if (hProcess == NULL) {
                LPCSTR szLastErrorMessage = MyGetLastErrorMessage();

                fprintf(stderr,
                        "Error: could not open the process with ID = %d, "
                        "called \"%s\". "
                        "Cause: %s", 
                        entry.th32ProcessID, 
                        entry.szExeFile,
                        szLastErrorMessage);

                free((void*) szLastErrorMessage);
                szLastErrorMessage = NULL;

            } else {
                if (TerminateProcess(hProcess, 0)) {
                    totalProcessesTerminated++;
                    printf("Terminated process ID %d\n", 
                            entry.th32ParentProcessID);
                } else {
                    LPCSTR szLastErrorMessage = MyGetLastErrorMessage();

                    fprintf(
                        stderr, 
                        "Warning: could not terminate the process with ID %d. "
                        "Cause: %s",
                        entry.th32ProcessID,
                        szLastErrorMessage);

                    free((void*) szLastErrorMessage);
                    szLastErrorMessage = NULL;
                }

                if (!CloseHandle(hProcess)) {
                    LPCSTR szLastErrorMessage = MyGetLastErrorMessage();

                    fprintf(
                        stderr,
                        "Warning: could not close the handle to the process ID %d. "
                        "Cause: %s",
                        entry.th32ProcessID,
                        szLastErrorMessage);

                    free((void*) szLastErrorMessage);
                    szLastErrorMessage = NULL;
                }
            }
        } while (Process32Next(snapshot, &entry));
    }

    BOOL snapshotHandleClosed = CloseHandle(snapshot);

    if (!snapshotHandleClosed) {
        LPCSTR szLastErrorMessage = MyGetLastErrorMessage();

        fprintf(stderr,
                "Warning: could not close the process snapshot. Cause: %s",
                szLastErrorMessage);

        free((void*) szLastErrorMessage);
        szLastErrorMessage = NULL;
    }

    printf("Info: total processes: %zu, "
           "total matching processes: %zu, total terminated: %zu.\n", 
           totalProcesses,
           totalProcessesMatched, 
           totalProcessesTerminated);

    return EXIT_SUCCESS;
}

Critique request

Please tell me anything that comes to mind.

Answer 1

Copying and pasting from my previous answer, your (feasible) options for error string memory management are

• Make the caller contract such that the caller is responsible for calling LocalFree, or
• Have get_last_err_msg() accept a buffer and size instead, and simply have FormatMessage fill that buffer

I made a mistake in offering the first without a warning on memory sections, since that's not going to work given your inconsistent use of string literals and heap-allocated strings. In your previous question this was an unobtrusive but still problematic ""; now it's a "no errors". You can easily get into hot water if a call fails but the error message ID is 0 - you're unconditionally freeing the string, and attempting to free a string literal is going to ruin your day.

So you can either:

• in the case of "no errors", conditionally allocate, strcpy and return your buffer; or
• do option 2 from above, which would be a strcpy only.