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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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?

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

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.

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Attachment: signature.asc
Description: This is a digitally signed message part