Re: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Jan 14, 2022 at 11:22:00PM +0000, Dario Faggioli wrote:
> On Thu, 2022-01-13 at 12:05 +0000, Anthony PERARD wrote:
> > On Wed, Jan 12, 2022 at 05:41:36PM +0100, Dario Faggioli wrote:
> > 
> > > 2) there should be nothing to free anyway
> > 
> > The issue here is that it doesn't appear to be true. Even if "info"
> > is
> > NULL, "nr" have an other value than 0, so libxl_vcpuinfo_list_free()
> > will try to access random addresses.
> > 
> My point here was that if vinfo is NULL (because libxl_list_vcpu()
> returned NULL), there aren't any list element or any list to free, so
> we can avoid calling libxl_vcpuinfo_list_free().
> 
> Then, sure, if we call it with a NULL vinfo and a non-zero
> nr_dom_vcpus, things explode, but this was being addressed in patch 2.
> 
> This first one was really only about not trying to free an empty list.
> And not to workaround the fact that it currently can make things
> explode, just because it feels pointless.
> 
> > > Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
> > > Tested-by: James Fehlig <jfehlig@xxxxxxxx>
> > 
> > Can I suggest to make libxl_vcpuinfo_list_free() work a bit better in
> > case it's "nr" parameter is wrong? It will do nothing if "list" is
> > NULL.
> >
> I thought about that, and we certainly can do it.
> 
> However, I think it's unrelated to this patch so, if I do it, I'll do
> it in its own one.
> 
> Also, if we go that way, I guess we want to change
> libxl_cputopology_list_free(), libxl_pcitopology_list_free(),
> libxl_numainfo_list_free(), libxl_dominfo_list_free(),
> libxl_vminfo_list_free() and libxl_cpupoolinfo_list_free(), don't we?

I actually don't know if that would be useful. Those functions do work
as expected (I think) with the right parameters: they do nothing when
called with (NULL, 0). free(NULL) does nothing.

So checking for NULL before using `nr` would probably just be a
workaround for programming mistake in the function allocating the object
or some missing initialisation in the caller. So I don't think it is
important anymore.

> > Also I think it is better to keep the free in the exit path at the
> > end
> > of the loop.
> > 
> Can I ask why as, as I was trying to say above, if we are sent directly
> to next by one of the goto, we do know that vinfo is NULL and
> libxl_vcpuinfo_list_free() will be basically a NOP, however it is
> implemented.
> 
> Of course, you're the maintainer of this code, so I'll do like that if
> you ask... I'm just curious. :-)

Freeing resources should always been attempted, even if that mean to
check whether there's something to free before calling the associated
free function. Imagine someone adding some code that could fail after
the libxl_list_vcpu(), then when that new code fails it would `goto
next;` and the allocated data from libxl_list_vcpu() would never be
freed.

> Actually, if you really think that the call to
> libxl_vcpuinfo_list_free() should stay where it is, I can just drop
> this patch and go on with patch 2 only, which is enough for fixing the
> crash.

This patch is just a workaround a bug in libxl_list_vcpu(), so I think
it can be dropped.

Cheers,

-- 
Anthony PERARD