A simple C WinAPI program for terminating processes via process image names - folllow-up 2

Question

(See also the previous follow-up.)

Now, I seem to improve my program partially via the answer in my previous follow-up. It goes like this:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <windows.h>
#include <TlHelp32.h>
/********************************************************************
* Searches the index of the last occurrence of the input character. *
********************************************************************/
static int get_last_char_index(
 const char* const arr,
 const int arr_len,
 const char c) {
 for (int i = arr_len - 1; i >= 0; i--) {
 if (arr[i] == c) {
 return i;
 }
 }
 return -1;
}
/************************************************************************
* Searches the index of the last occurrence of the backslash character. *
************************************************************************/
static int get_last_backslash_index(
 const char* const arr,
 const int arr_len) {
 return get_last_char_index(arr, arr_len, '\\');
}
/***********************************************
* Returns the base name of this process image. *
***********************************************/
static char* get_base_name(const char* const arg) {
 size_t arg_len = strlen(arg);
 int backslash_char_index = get_last_backslash_index(arg, arg_len);
 size_t return_char_array_len = arg_len - backslash_char_index;
 char* carr = (char*)calloc(return_char_array_len, sizeof(char));
 carr[return_char_array_len - 1] = NULL;
 memcpy(carr, &arg[backslash_char_index + 1], return_char_array_len);
 return carr;
}
int main(int argc, char* argv[]) {
 if (argc != 2) {
 char* base_name = get_base_name(argv[0]);
 fprintf(stderr, "%s PROCESS_NAME\n", base_name);
 free(base_name);
 return EXIT_FAILURE;
 }
 PROCESSENTRY32 entry;
 entry.dwSize = sizeof(PROCESSENTRY32);
 HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, NULL);
 if (snapshot == INVALID_HANDLE_VALUE) {
 fputs("Error: could not get the process snapshot.\n", stderr);
 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\".\n", 
 entry.th32ProcessID, 
 entry.szExeFile);
 } else {
 BOOL terminated = TerminateProcess(hProcess, 0);
 if (terminated) {
 totalProcessesTerminated++;
 BOOL closed = CloseHandle(hProcess);
 if (!closed) {
 fprintf(stderr,
 "Warning: could not close a handle "
 "for process ID = %d, called \"%s\".\n",
 entry.th32ProcessID, 
 entry.szExeFile);
 }
 printf("Terminated process ID %d\n", entry.th32ProcessID);
 }
 }
 }
 } while (Process32Next(snapshot, &entry));
 }
 BOOL snapshotHandleClosed = CloseHandle(snapshot);
 if (!snapshotHandleClosed) {
 fputs("Warning: could not close the process snapshot.", stderr);
 }
 printf("Info: total processes: %zu, "
 "total matching processes: %zu, total terminated: %zu.\n", 
 totalProcesses,
 totalProcessesMatched, 
 totalProcessesTerminated);
 return EXIT_SUCCESS;
}

Critique request

I am eager to hear any comments whatsoever.

Answer 1

Some remarks with respect to the get_base_name() function:

• size_t vs int: You correctly start with

  size_t arg_len = strlen(arg);
  

  but then pass arg_len to get_last_backslash_index() which takes an int argument. Depending on the strictness of your compiler this can cause a "Implicit conversion loses integer precision" warning. I suggest to use size_t consequently for string length.

• The cast in

  char* carr = (char*)calloc(return_char_array_len, sizeof(char));
  

  is not needed and generally not recommended, see for example Do I cast the result of malloc? on Stack Overflow.

• sizeof(char) is always equal to one, therefore it can be shortened to

  char* carr = malloc(return_char_array_len);
  

• Here

  carr[return_char_array_len - 1] = NULL;
  

  a pointer value is assigned to char (which is an integral type). The right-hand side should be 0 or '0円' to avoid compiler warnings.

• The implementation can be shortened if you take advantage of Microsoft C runtime library functions like strrchr() and _strdup():

  static char* get_base_name(const char* const arg) {
   const char *backslash_char_ptr = strrchr(arg, '\\');
   return _strdup(backslash_char_ptr == NULL ? arg : backslash_char_ptr + 1);
  }
  

• I would perhaps call the function something like copy_base_name() to make it more obvious that the caller is responsible for releasing the memory.

• As an alternative you can use existing functions from the Microsoft C runtime like splitpath, or Shell Path Handling Functions like PathStripPath().

Some more remarks:

• No error message is printed if terminating a process failed.
• The process handle is closed only if terminating the process succeeded.

I would perhaps do it like this:

HANDLE hProcess = OpenProcess(...);
if (hProcess == NULL) {
 // Print error message
} else {
 if (TerminateProcess(hProcess, 0)) {
 totalProcessesTerminated++;
 printf("Terminated process ID %d\n", entry.th32ProcessID);
 } else {
 // Print error message
 }
 if (!CloseHandle(hProcess)) {
 // Print error message
 }
}

In all error situations it would be useful to print the actual reason for the failure (e.g. "access denied"). This can be achieved with GetLastError() and FormatMessage(), see for example How to get the error message from the error code returned by GetLastError()? on Stack Overflow.

• pacmaninbw: Visual Studio C requires _CRT_SECURE_NO_WARNINGS to use strdup(). They support _strdup() instead.
• Martin R: @pacmaninbw: You are right (I had compiled this with Clang on a Mac :), thanks for the notice.
• Martin R: @pacmaninbw: I also wrongly said that strdup() is a C standard library function (which is isn't – yet). Apparently that is the reason why Microsoft prepends an underscore to the function name.
• pacmaninbw: Yes, it has been added to a future standard but not in the current one. See this SO question. However, you were correct about strrchr.