Re: [PATCH,Hurd] Fix deallocation after proc_getprocinfo call

[Joel Brobecker <brobecker@adacore.com>]

Hello Samuel,

On Sun, Nov 02, 2014 at 04:25:37PM +0100, Samuel Thibault wrote:
> 2014-10-02  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
> 	* gdb/gnu-nat.c (inf_validate_procinfo): Multiply the number of
> 	elements pi_len by the size of the elements before calling
> 	vm_deallocate.
>         (inf_validate_task_sc): Likewise, and properly deallocate the
>         noise array.

Again, sorry about the late review...

I only have a few minor comments, almost trivial in nature. In
the ChangeLog entry above, watch out that the last 2 lines are
indented using spaces intead of tabs.

> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
> index d17a750..c571190 100644
> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -804,7 +804,7 @@ inf_validate_procinfo (struct inf *inf)
>        inf->nomsg = !!(pi->state & PI_NOMSG);
>        if (inf->nomsg)
>  	inf->traced = !!(pi->state & PI_TRACED);
> -      vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len);
> +      vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len * sizeof (*(procinfo_t) 0));

The line is too long (soft limit is 74 characters, hard limit is 80).
Suggest using "sizeof (struct procinfo)", which I think is better
than dereferencing a NULL pointer. This is based on guessing that
type procinfo_t is a pointer to struct procinfo, as suggested by
the code in inf_validate_procinfo.

>        if (noise_len > 0)
>  	vm_deallocate (mach_task_self (), (vm_address_t) noise, noise_len);
>      }
> @@ -844,9 +844,9 @@ inf_validate_task_sc (struct inf *inf)
>  
>    suspend_count = pi->taskinfo.suspend_count;
>  
> -  vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len);
> +  vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len * sizeof (*(procinfo_t) 0));

Same as above.

>    if (noise_len > 0)
> -    vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len);
> +    vm_deallocate (mach_task_self (), (vm_address_t) noise, noise_len);
>  
>    if (inf->task->cur_sc < suspend_count)
>      {

Thank you,
-- 
Joel