Review | endproc.c: Implement ProcessPage_OnEndProcessTree

[Henri Verbeet]

On 20 April 2017 at 20:10, Akarsha Sehwag <akarsha15010 at iiitd.ac.in> wrote:
> Hi
> I hope this is more readable and easier to review.
>
It looks like your mail client wrapped the patch. When using webmail
in particular it's probably better to just attach the patch, although
using git send-email would be best.

I think the patch is largely ok, but do have a few comments.

> +static void 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;
> +}
HEAP_ZERO_MEMORY is unnecessary, since "pid" entries are always
initialised before being used in process_list_append().

> +static void increase_list_size(process_list *list) {
> +    list->size *= 2;
> +    list->pid = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, list->pid,
> list->size * sizeof(*(list->pid)));
> +}
Although unlikely, HeapReAlloc() can fail and return NULL. It would be
better to handle that possibility. Since increase_list_size() and
process_list_append() are both small functions, I think it's worth
considering merging increase_list_size() into process_list_append(),
since that would probably simplify handling HeapReAlloc() failures.
The comment about HEAP_ZERO_MEMORY in init_process_list() applies here
as well.

> @@ -125,20 +187,40 @@ void ProcessPage_OnEndProcessTree(void)
>      if (MessageBoxW(hMainWnd, wszWarnMsg, wszWarnTitle,
> MB_YESNO|MB_ICONWARNING) != IDYES)
>          return;
>
> -    hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, dwProcessId);
>
> -    if (!hProcess)
> -    {
> -        GetLastErrorText(wstrErrorText,
> sizeof(wstrErrorText)/sizeof(WCHAR));
> -        MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate,
> MB_OK|MB_ICONSTOP);
> +    init_process_list(&list);
> +
> +    if(list.pid == NULL)
>          return;
Although not necessarily wrong, I think checking "list.pid" here is a
little awkward, since the possibility of it being NULL is an
implementation detail of init_process_list(). I think it would be
better to either make init_process_list() return an error and handle
that here, or just inline init_process_list() and free_process_list()
into their callers.

> +    snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS,
> 0);//TH32SNAPPROCESS
> +
> +    if(!snapshot)
> +        return;
CreateToolhelp32Snapshot() returns INVALID_HANDLE_VALUE instead of
NULL on failure. Returning here would leak "list.pid". Also, there
seems to be a leftover comment.

> +    for(i = 0; i < list.count; ++i) {
> +        hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, list.pid[i]);
> +
> +        if (!hProcess)
> +        {
> +            GetLastErrorText(wstrErrorText,
> sizeof(wstrErrorText)/sizeof(WCHAR));
> +            MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate,
> MB_OK|MB_ICONSTOP);
> +            break;
> +        }
> +
> +        if (!TerminateProcess(hProcess, 0))
> +        {
> +            GetLastErrorText(wstrErrorText,
> sizeof(wstrErrorText)/sizeof(WCHAR));
> +            MessageBoxW(hMainWnd, wstrErrorText,wszUnable2Terminate,
> MB_OK|MB_ICONSTOP);
> +        }
> +        CloseHandle(hProcess);
>      }
It may be worth considering introducing a function to terminate a
process here. I.e.:

    if (!kill_process(list.pid[i]))
    {
       GetLastErrorText(error_text, sizeof(error_text) / sizeof(*error_text));
       MessageBoxW(hMainWnd, error_text, wszUnable2Terminate, MB_OK |
MB_ICONSTOP);
       break;
    }

Such a function could be used by ProcessPage_OnEndProcess() as well.

Henri