1
\$\begingroup\$

(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.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.