2
\$\begingroup\$

(See the next follow-up here.)

This is the 4th version of the program killer tool:

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

static const char* get_last_err_msg() {
    DWORD errorMessageId = GetLastError();

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

    LPSTR 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,
            0,
            NULL);

    char* errmsg = _strdup(messageBuffer);
    LocalFree(messageBuffer);
    return errmsg;
}

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) {
        fprintf(
            stderr,
            "Error: could not get the process snapshot. "
            "Cause: %s\n", get_last_err_msg());
        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) {
                totalProcessesMatched++;

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

                if (hProcess == NULL) {
                    fprintf(stderr,
                            "Error: could not open the process with ID = %d, "
                            "called \"%s\". "
                            "Cause: %s", 
                            entry.th32ProcessID, 
                            entry.szExeFile,
                            get_last_err_msg());
                } else {
                    if (TerminateProcess(hProcess, 0)) {
                        totalProcessesTerminated++;
                        printf("Terminated process ID %d\n", 
                                entry.th32ParentProcessID);
                    } else {
                        fprintf(
                            stderr, 
                            "Warning: could not terminate the process with ID %d. "
                            "Cause: %s",
                            entry.th32ProcessID,
                            get_last_err_msg());
                    }

                    if (!CloseHandle(hProcess)) {
                        fprintf(
                            stderr,
                            "Warning: could not close the handle to the process ID %d. "
                            "Cause: %s",
                            entry.th32ProcessID,
                            get_last_err_msg());
                    }
                }
            }
        } while (Process32Next(snapshot, &entry));
    }

    BOOL snapshotHandleClosed = CloseHandle(snapshot);

    if (!snapshotHandleClosed) {
        fprintf(stderr,
                "Warning: could not close the process snapshot. Cause: %s",
                get_last_err_msg());
    }

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

    return EXIT_SUCCESS;
}

Critique request

Is there anything else I could do here?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Memory ownership

This:

char* errmsg = _strdup(messageBuffer);
LocalFree(messageBuffer);
return errmsg;

is problematic. The documentation for _strdup says that

The _strdup function calls malloc to allocate storage space for a copy of strSource and then copies strSource to the allocated space.

So you're allocating, reallocating, doing one free, and leaving the second buffer dangling. The typical solutions for this are

  • Make the caller contract such that the caller is responsible for calling LocalFree, and don't call strdup at all. If you do this, the "" case must use your own malloc and strcpy.
  • Have get_last_err_msg() accept a buffer and size instead, and simply have FormatMessage fill that buffer
  • Accept a callback function pointer that is called with the message, after which get_last_err_msg() does the free - this one is needlessly complicated for this application

I recommend the second.

Logic inversion

Change this:

        if (strcmp(entry.szExeFile, argv[1]) == 0) {

to

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

so that the rest of the loop can be de-indented.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I don't agree about adding continue, the need to do that always originates from cluttered, needlessly complex loops. In this case the solution is to split the big function into several. The OpenProcess + handling the results from it looks like a good candidate for a separate function. \$\endgroup\$
    – Lundin
    Oct 14, 2020 at 6:20

Your Answer

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

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