Review | endproc.c: Implement ProcessPage_OnEndProcessTree

[Bruno Jesus]

On Thu, Apr 27, 2017 at 9:11 AM, Akarsha Sehwag
<akarsha15010 at iiitd.ac.in> wrote:
> ...
>
> I didn't understand the function of the flags in HeapAlloc() and
> HeapReAlloc(). I read the function RtlAllocateHeap() but I am unable to
> figure out which flag to use out of HEAP_GENERATE_EXCEPTIONS,
> HEAP_NO_SERIALIZE and HEAP_ZERO_MEMORY.

When no flags are required you can always use 0.

> diff --git a/programs/taskmgr/endproc.c b/programs/taskmgr/endproc.c
> index 89c2d7b..c890f06 100644
> --- a/programs/taskmgr/endproc.c
> +++ b/programs/taskmgr/endproc.c
> @@ -31,17 +31,42 @@
>  #include "wine/unicode.h"
>  #include "taskmgr.h"
>  #include "perfdata.h"
> +#include "tlhelp32.h"
> +
>

Please avoid double empty line.

>  static WCHAR wszWarnMsg[511];
>  static WCHAR wszWarnTitle[255];
>  static WCHAR wszUnable2Terminate[255];
>
> +typedef struct process_list {
> +    DWORD        *pid;       /*dynamic array to store the process IDs*/
> +    SIZE_T      count;      /*index to maintain the last entry of the array;*/
> +    SIZE_T      size;       /*the current size of the pid array*/
> +} process_list;

Please use a space between /* */ and the comments, but anyway the
struct above is pretty self explanatory so there is no need to comment
each field.

> +
>  static void load_message_strings(void)
>  {
>      LoadStringW(hInst, IDS_TERMINATE_MESSAGE, wszWarnMsg, sizeof(wszWarnMsg)/sizeof(WCHAR));
>      LoadStringW(hInst, IDS_TERMINATE_UNABLE2TERMINATE, wszUnable2Terminate, sizeof(wszUnable2Terminate)/sizeof(WCHAR));
>      LoadStringW(hInst, IDS_WARNING_TITLE, wszWarnTitle, sizeof(wszWarnTitle)/sizeof(WCHAR));
>  }
> +
> +static void kill_process(HANDLE hProcess, WCHAR *wstrErrorText)
> +{
> +    if (!hProcess)
> +    {
> +        GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));
> +        MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
> +        return;
> +    }
> +
> +    if (!TerminateProcess(hProcess, 0))
> +    {
> +        GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));
> +        MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
> +    }
> +}
> +

I known this was copy pasted from original code but please fix the
missing space for ",wszUnable2Terminate".

>  void ProcessPage_OnEndProcess(void)
>  {
> @@ -77,22 +102,71 @@ void ProcessPage_OnEndProcess(void)
>
>      hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, dwProcessId);
>
> -    if (!hProcess)
> -    {
> -        GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));
> -        MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
> -        return;
> +
> +    kill_process(hProcess, wstrErrorText);
> +
> +    CloseHandle(hProcess);
> +}
> +
> +

Double empty line.

> +static INT init_process_list(process_list *list) {
> +    list->size = 4;            /*initialise size with 4. Will increase if necessary.*/
> +    list->pid = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, list->size * sizeof(*(list->pid)));
> +    list->count = 0;
> +
> +    if(list->pid == NULL)
> +        return 0;
> +
> +    return 1;
> +}

Just change the HEAP_ZERO_MEMORY for 0. There is no need for extra
parenthesis at "sizeof(*(list->pid))", so "sizeof(*list->pid)".
Make the function BOOL and "return list->pid != NULL" instead of
double return 0/1.

> +
> +static INT process_list_append(process_list *list, DWORD id) {
> +    if(list->count == list->size){
> +        list->size *= 2;
> +        list->pid = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, list->pid, list->size * sizeof(*(list->pid)));
> +        if(!list->pid)
> +            return 0;
>      }

The rest of the code uses { in the next line below the if, please do
the same here. And add the missing space from "if(" to "if (".
You need an intermediate variable to store the results of HeapReAlloc
otherwise you will lose the original pointer in case of error.

> +    list->pid[list->count] = id;
> +    list->count += 1;
> +    return 1;
> +}

Make the function BOOL. Minor style nitpick: use list->count++. You
can also merge both lines: list->pid[list->count++] = id;

>
> -    if (!TerminateProcess(hProcess, 0))
> +static void free_process_list(process_list *list) {
> +    HeapFree(GetProcessHeap(), 0, list->pid);
> +}
> +
> +

Double empty line.

> +static void enum_process_children(HANDLE snapshot, process_list *list, DWORD pid) {
> +    PROCESSENTRY32 entry;
> +
> +    SIZE_T start, end, i;

"{" in the next line. Remove the empty line between variable declarations.

> +
> +    start = list->count;
> +
> +    entry.dwSize = sizeof(entry);
> +
> +    if(!Process32First(snapshot, &entry))
> +        return;
> +
> +    do
> +    {
> +        if(entry.th32ParentProcessID == pid)
> +        {
> +            if(!process_list_append(list, entry.th32ProcessID))
> +                return;
> +        }
> +    } while (Process32Next(snapshot, &entry));
> +
> +    end = list->count;
> +
> +    for(i = start; i < end; ++i)
>      {
> -        GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));
> -        MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
> +        enum_process_children(snapshot, list, list->pid[i]);
>      }
> -
> -    CloseHandle(hProcess);
>  }
>
> +

Double empty line.

>  void ProcessPage_OnEndProcessTree(void)
>  {
>      LVITEMW          lvitem;
> @@ -100,6 +174,9 @@ void ProcessPage_OnEndProcessTree(void)
>      DWORD            dwProcessId;
>      HANDLE           hProcess;
>      WCHAR            wstrErrorText[256];
> +    process_list     list;
> +    SIZE_T           i;
> +    HANDLE           snapshot;
>
>      load_message_strings();
>
> @@ -125,20 +202,32 @@ void ProcessPage_OnEndProcessTree(void)
>      if (MessageBoxW(hMainWnd, wszWarnMsg, wszWarnTitle, MB_YESNO|MB_ICONWARNING) != IDYES)
>          return;
>
> -    hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, dwProcessId);
>
> -    if (!hProcess)
> +    if(!init_process_list(&list))
> +        return;
> +
> +    if(!process_list_append(&list, dwProcessId))
> +        return;

You are not freeing the list in case of error here.

> +
> +    snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
> +
> +    if(snapshot == INVALID_HANDLE_VALUE)
>      {
> -        GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));
> -        MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
> +        free_process_list(&list);
>          return;
>      }
>
> -    if (!TerminateProcess(hProcess, 0))
> -    {
> -        GetLastErrorText(wstrErrorText, sizeof(wstrErrorText)/sizeof(WCHAR));
> -        MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate, MB_OK|MB_ICONSTOP);
> +    enum_process_children(snapshot, &list, dwProcessId);
> +
> +    CloseHandle(snapshot);
> +
> +    for(i = 0; i < list.count; ++i) {
> +        hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, list.pid[i]);
> +
> +        kill_process(hProcess, wstrErrorText);
> +
> +        CloseHandle(hProcess);
>      }

"{" in the next line, no need for empty lines between functions.

>
> -    CloseHandle(hProcess);
> +    free_process_list(&list);
>  }
>

Watch out for trailing space issues:

/home/bruno/Downloads/diff.txt:9: trailing whitespace.
#include "tlhelp32.h"
/home/bruno/Downloads/diff.txt:28: trailing whitespace.

/home/bruno/Downloads/diff.txt:43: trailing whitespace.
}
/home/bruno/Downloads/diff.txt:57: trailing whitespace.

/home/bruno/Downloads/diff.txt:100: trailing whitespace.
    entry.dwSize = sizeof(entry);
warning: squelched 5 whitespace errors
warning: 10 lines add whitespace errors.

Best wishes,
Bruno