Re: Review | endproc.c: Implement ProcessPage_OnEndProcessTree

[Henri Verbeet]

On 20 April 2017 at 20:10, Akarsha Sehwag  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