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

[Elliott Mitchell <ehem+xen@xxxxxxx>]

Huh, never got around to replying to this.  Too many things going on, too
many distractions...

On Wed, Jan 05, 2022 at 03:05:23PM +0000, Anthony PERARD wrote:
> 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.

I disagree with this statement.

> 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().

I agree with this statement.

> "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.

That may be what the author was thinking when they wrote lines 579 & 580.
Problem is info_free is a pointer to libxl_dominfo, *not* a boolean.

> 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?

What you're describing seems to be precisely what the patch does.
Perhaps you got the roles of "info" and "info_free" reversed?

This actually points to an issue on lines 548 & 553.  Instead of storing
the return from libxl_list_domain() into "info" then copying to
"info_free" both should be set at the same time.

I had noticed this (and cringed), but didn't feel it was currently
worthwhile to go after lines 548 & 553.  If you want this additional
change to accept the patch, I'm up for that.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445