If libxl_vcpu_list() returned NULL, we should not call
libxl_numainfo_list_free() as:
1) it'll fail trying to (double) free() *list
2) there should be nothing to free anyway
Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
Tested-by: James Fehlig <jfehlig@suse.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
---
tools/libs/light/libxl_numa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libs/light/libxl_numa.c b/tools/libs/light/libxl_numa.c
index a8a75f89e9..3679028c79 100644
--- a/tools/libs/light/libxl_numa.c
+++ b/tools/libs/light/libxl_numa.c
@@ -253,9 +253,9 @@ static int nr_vcpus_on_nodes(libxl__gc *gc, libxl_cputopology *tinfo,
}
}
+ libxl_vcpuinfo_list_free(vinfo, nr_dom_vcpus);
next:
libxl_cpupoolinfo_dispose(&cpupool_info);
- libxl_vcpuinfo_list_free(vinfo, nr_dom_vcpus);
}
libxl_bitmap_dispose(&dom_nodemap);
On Wed, Jan 12, 2022 at 05:41:36PM +0100, Dario Faggioli wrote: > If libxl_vcpu_list() returned NULL, we should not call > libxl_numainfo_list_free() as: You mean libxl_vcpuinfo_list_free() ? > 1) it'll fail trying to (double) free() *list This isn't really an issue. free(NULL) is legit, can be call as many time as you want. > 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. > Signed-off-by: Dario Faggioli <dfaggioli@suse.com> > Tested-by: James Fehlig <jfehlig@suse.com> 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. Even if that seems wrong, and the caller should use the correct value. Also I think it is better to keep the free in the exit path at the end of the loop. Thanks, -- Anthony PERARD
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@suse.com> > > Tested-by: James Fehlig <jfehlig@suse.com> > > 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)
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@suse.com> > > > Tested-by: James Fehlig <jfehlig@suse.com> > > > > 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
On Mon, 2022-01-17 at 15:56 +0000, Anthony PERARD wrote: > On Fri, Jan 14, 2022 at 11:22:00PM +0000, Dario Faggioli wrote: > > > > 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. > Right. > 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. > Agreed. That's why, as I said, I though about doing something like that, but ended up deciding not to. > > > 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. > Yeah, sure, whoever adds code that does a 'goto next' with an allocated list, should also realize that libxl_vcpuinfo_list_free() needs to be moved after 'next' itself, as a consequence of the very change being done, and this seems fair to me. But, at the end of the day, it's not a big deal at all. Thanks for satisfying my curiosity and, since you also agree on... > > 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. > ...this, I'll just drop this patch and proceed with just what, in this series, was patch 2. 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)
© 2016 - 2024 Red Hat, Inc.