Re: [PATCH 5/5] tools/xl: Fix potential deallocation bug

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Dec 10, 2020 at 03:09:06PM -0800, Elliott Mitchell wrote:
> There is potential for the info and info_free variable's purposes to
> diverge.  If info was overwritten with a distinct value, yet info_free
> still needed deallocation a bug would occur on this line.  Preemptively
> address this issue (making use of divergent info/info_free values is
> under consideration).
> 
> Signed-off-by: Elliott Mitchell <ehem+xen@xxxxxxx>
> ---
>  tools/xl/xl_info.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 3647468420..938f06f1a8 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -579,7 +579,7 @@ int main_list(int argc, char **argv)
>                       info, nb_domain);
>  
>      if (info_free)
> -        libxl_dominfo_list_free(info, nb_domain);
> +        libxl_dominfo_list_free(info_free, nb_domain);
>  
>      libxl_dominfo_dispose(&info_buf);
>  

I don't think this is the right thing to do with this patch.
libxl_dominfo_list_free() should use the same variable that is used by
libxl_list_domain(). What we want to free is the allocation made by
libxl_list_domain().

"info_free" in the function seems to be used as a boolean which tell
if "info" have been allocated or not. Actually, it probably say if
"info" is a list of "libxl_dominfo" or not.

So instead of just replacing "info" by "info_free" here, we should
instead store the result from libxl_list_domain() into a different
variable and free that, like it is done with "info_buf".

I hope that makes sense?

Thanks,

-- 
Anthony PERARD