| Message ID | 20240520164400.15740-1-leigh@solinno.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/libs/light: Fix nic->vlan memory allocation | expand |
On 2024-05-20 12:44, Leigh Brown wrote: > After the following commit: > 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") > xl list -l aborts with a double free error if a domain has at least > one vif defined: > > $ sudo xl list -l > free(): double free detected in tcache 2 > Aborted > > Orginally, the vlan field was called vid and was defined as an integer. > It was appropriate to call libxl__xs_read_checked() with gc passed as > the string data was copied to a different variable. However, the final > version uses a string data type and the call should have been changed > to use NOGC instead of gc to allow that data to live past the gc > controlled lifetime, in line with the other string fields. > > This patch makes the change to pass NOGC instead of gc and moves the > new code to be next to the other string fields (fixing a couple of > errant tabs along the way), as recommended by Jason. > > Fixes: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") > Signed-off-by: Leigh Brown <leigh@solinno.co.uk> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Thanks, Jason
On 20.05.2024 19:08, Jason Andryuk wrote: > On 2024-05-20 12:44, Leigh Brown wrote: >> After the following commit: >> 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") >> xl list -l aborts with a double free error if a domain has at least >> one vif defined: >> >> $ sudo xl list -l >> free(): double free detected in tcache 2 >> Aborted >> >> Orginally, the vlan field was called vid and was defined as an integer. >> It was appropriate to call libxl__xs_read_checked() with gc passed as >> the string data was copied to a different variable. However, the final >> version uses a string data type and the call should have been changed >> to use NOGC instead of gc to allow that data to live past the gc >> controlled lifetime, in line with the other string fields. >> >> This patch makes the change to pass NOGC instead of gc and moves the >> new code to be next to the other string fields (fixing a couple of >> errant tabs along the way), as recommended by Jason. >> >> Fixes: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") >> Signed-off-by: Leigh Brown <leigh@solinno.co.uk> > > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> I notice this wasn't Cc-ed to the maintainer, which likely is the reason for there not having been an ack yet. Anthony, any thoughts? Further at this point, bug fix or not, it would likely also need a release ack. Oleksii, thoughts? Jan
On Mon, May 20, 2024 at 01:08:03PM -0400, Jason Andryuk wrote: > On 2024-05-20 12:44, Leigh Brown wrote: > > After the following commit: > > 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") > > xl list -l aborts with a double free error if a domain has at least > > one vif defined: > > > > $ sudo xl list -l > > free(): double free detected in tcache 2 > > Aborted > > > > Orginally, the vlan field was called vid and was defined as an integer. > > It was appropriate to call libxl__xs_read_checked() with gc passed as > > the string data was copied to a different variable. However, the final > > version uses a string data type and the call should have been changed > > to use NOGC instead of gc to allow that data to live past the gc > > controlled lifetime, in line with the other string fields. > > > > This patch makes the change to pass NOGC instead of gc and moves the > > new code to be next to the other string fields (fixing a couple of > > errant tabs along the way), as recommended by Jason. > > > > Fixes: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") > > Signed-off-by: Leigh Brown <leigh@solinno.co.uk> > > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Acked-by: Anthony PERARD <anthony.perard@vates.tech> Thanks,
On Wed, 2024-06-19 at 13:57 +0200, Jan Beulich wrote: > On 20.05.2024 19:08, Jason Andryuk wrote: > > On 2024-05-20 12:44, Leigh Brown wrote: > > > After the following commit: > > > 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to > > > libxl_device_nic") > > > xl list -l aborts with a double free error if a domain has at > > > least > > > one vif defined: > > > > > > $ sudo xl list -l > > > free(): double free detected in tcache 2 > > > Aborted > > > > > > Orginally, the vlan field was called vid and was defined as an > > > integer. > > > It was appropriate to call libxl__xs_read_checked() with gc > > > passed as > > > the string data was copied to a different variable. However, the > > > final > > > version uses a string data type and the call should have been > > > changed > > > to use NOGC instead of gc to allow that data to live past the gc > > > controlled lifetime, in line with the other string fields. > > > > > > This patch makes the change to pass NOGC instead of gc and moves > > > the > > > new code to be next to the other string fields (fixing a couple > > > of > > > errant tabs along the way), as recommended by Jason. > > > > > > Fixes: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to > > > libxl_device_nic") > > > Signed-off-by: Leigh Brown <leigh@solinno.co.uk> > > > > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> > > I notice this wasn't Cc-ed to the maintainer, which likely is the > reason > for there not having been an ack yet. Anthony, any thoughts? > > Further at this point, bug fix or not, it would likely also need a > release > ack. Oleksii, thoughts? It seems to me it is bug fix, so it should be in release: Release-acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> ~ Oleksii
On 19.06.2024 14:57, Anthony PERARD wrote: > On Mon, May 20, 2024 at 01:08:03PM -0400, Jason Andryuk wrote: >> On 2024-05-20 12:44, Leigh Brown wrote: >>> After the following commit: >>> 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") >>> xl list -l aborts with a double free error if a domain has at least >>> one vif defined: >>> >>> $ sudo xl list -l >>> free(): double free detected in tcache 2 >>> Aborted >>> >>> Orginally, the vlan field was called vid and was defined as an integer. >>> It was appropriate to call libxl__xs_read_checked() with gc passed as >>> the string data was copied to a different variable. However, the final >>> version uses a string data type and the call should have been changed >>> to use NOGC instead of gc to allow that data to live past the gc >>> controlled lifetime, in line with the other string fields. >>> >>> This patch makes the change to pass NOGC instead of gc and moves the >>> new code to be next to the other string fields (fixing a couple of >>> errant tabs along the way), as recommended by Jason. >>> >>> Fixes: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") >>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk> >> >> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> > > Acked-by: Anthony PERARD <anthony.perard@vates.tech> Btw, at the example of this: Are you meaning to update ./MAINTAINERS with that new email address of yours. Strictly speaking I think for Acked-by: to actually fulfill its purpose (and for R-b to have its normally implied meaning of "ack" when coming from a maintainer), it probably ought to match the corresponding entry in ./MAINTAINERS. Jan
diff --git a/tools/libs/light/libxl_nic.c b/tools/libs/light/libxl_nic.c index d861e3726d..300a96a8b1 100644 --- a/tools/libs/light/libxl_nic.c +++ b/tools/libs/light/libxl_nic.c @@ -318,11 +318,6 @@ static int libxl__nic_from_xenstore(libxl__gc *gc, const char *libxl_path, nic->mtu = LIBXL_DEVICE_NIC_MTU_DEFAULT; } - rc = libxl__xs_read_checked(gc, XBT_NULL, - GCSPRINTF("%s/vlan", libxl_path), - (const char **)(&nic->vlan)); - if (rc) goto out; - rc = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/mac", libxl_path), &tmp); if (rc) goto out; @@ -345,6 +340,10 @@ static int libxl__nic_from_xenstore(libxl__gc *gc, const char *libxl_path, GCSPRINTF("%s/script", libxl_path), (const char **)(&nic->script)); if (rc) goto out; + rc = libxl__xs_read_checked(NOGC, XBT_NULL, + GCSPRINTF("%s/vlan", libxl_path), + (const char **)(&nic->vlan)); + if (rc) goto out; rc = libxl__xs_read_checked(NOGC, XBT_NULL, GCSPRINTF("%s/forwarddev", libxl_path), (const char **)(&nic->coloft_forwarddev));
After the following commit: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") xl list -l aborts with a double free error if a domain has at least one vif defined: $ sudo xl list -l free(): double free detected in tcache 2 Aborted Orginally, the vlan field was called vid and was defined as an integer. It was appropriate to call libxl__xs_read_checked() with gc passed as the string data was copied to a different variable. However, the final version uses a string data type and the call should have been changed to use NOGC instead of gc to allow that data to live past the gc controlled lifetime, in line with the other string fields. This patch makes the change to pass NOGC instead of gc and moves the new code to be next to the other string fields (fixing a couple of errant tabs along the way), as recommended by Jason. Fixes: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to libxl_device_nic") Signed-off-by: Leigh Brown <leigh@solinno.co.uk> --- tools/libs/light/libxl_nic.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)