If @vm has flagged as "to be removed" virDomainObjListFindByNameLocked
returns NULL (although the definition actually exists). Therefore, the
possibility exits that "virHashAddEntry" will raise the error
"Duplicate key" => virDomainObjListAddObjLocked fails =>
virDomainObjEndAPI(&vm) is called and this leads to a freeing of @def
since @def is already assigned to vm->def. But actually this leads to
a double free since the common usage pattern is that the caller of
virDomainObjListAdd(Locked) is responsible for freeing @def in case of
an error.
Let's fix this by setting vm->def to NULL in case of an error.
Backtrace:
➤ bt
#0 virFree (ptrptr=0x7575757575757575)
#1 0x000003ffb5b25b3e in virDomainResourceDefFree
#2 0x000003ffb5b37c34 in virDomainDefFree
#3 0x000003ff9123f734 in qemuDomainDefineXMLFlags
#4 0x000003ff9123f7f4 in qemuDomainDefineXML
#5 0x000003ffb5cd2c84 in virDomainDefineXML
#6 0x000000011745aa82 in remoteDispatchDomainDefineXML
...
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
src/conf/virdomainobjlist.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 52171594f34f..805fe9440afe 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -329,8 +329,10 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
goto cleanup;
vm->def = def;
- if (virDomainObjListAddObjLocked(doms, vm) < 0)
+ if (virDomainObjListAddObjLocked(doms, vm) < 0) {
+ vm->def = NULL;
goto error;
+ }
}
cleanup:
return vm;
--
2.13.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/27/2018 03:20 PM, Marc Hartmayer wrote: > If @vm has flagged as "to be removed" virDomainObjListFindByNameLocked > returns NULL (although the definition actually exists). Therefore, the > possibility exits that "virHashAddEntry" will raise the error > "Duplicate key" => virDomainObjListAddObjLocked fails => > virDomainObjEndAPI(&vm) is called and this leads to a freeing of @def > since @def is already assigned to vm->def. But actually this leads to > a double free since the common usage pattern is that the caller of > virDomainObjListAdd(Locked) is responsible for freeing @def in case of > an error. > > Let's fix this by setting vm->def to NULL in case of an error. > > Backtrace: > > ➤ bt > #0 virFree (ptrptr=0x7575757575757575) > #1 0x000003ffb5b25b3e in virDomainResourceDefFree > #2 0x000003ffb5b37c34 in virDomainDefFree > #3 0x000003ff9123f734 in qemuDomainDefineXMLFlags > #4 0x000003ff9123f7f4 in qemuDomainDefineXML > #5 0x000003ffb5cd2c84 in virDomainDefineXML > #6 0x000000011745aa82 in remoteDispatchDomainDefineXML > ... > > Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> > Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > --- > src/conf/virdomainobjlist.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c > index 52171594f34f..805fe9440afe 100644 > --- a/src/conf/virdomainobjlist.c > +++ b/src/conf/virdomainobjlist.c > @@ -329,8 +329,10 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, > goto cleanup; > vm->def = def; > > - if (virDomainObjListAddObjLocked(doms, vm) < 0) > + if (virDomainObjListAddObjLocked(doms, vm) < 0) { > + vm->def = NULL; > goto error; > + } > } > cleanup: > return vm; > How about setting vm->def only after virDomainObjListAddObjLocked() succeded? However, this points to a more sever bug. If a domain is being removed, and some other thread is trying to define it back, I guess the whole operation should succeed. Definitely not fail with some (for user) cryptic message like "double key found in a hash table". The problem is that: a) both virDomainObjListFindByUUIDLocked() and virDomainObjListFindByNameLocked() return NULL even if domain exists. They should return the found domain and only the caller should decide if vm->removing is relevant or not. b) nothing is clearing out the vm->removing flag. The virDomainObjListAddObjLocked() looks like a good candidate to do so. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Aug 27, 2018 at 04:03 PM +0200, Michal Prívozník <mprivozn@redhat.com> wrote: > On 08/27/2018 03:20 PM, Marc Hartmayer wrote: >> If @vm has flagged as "to be removed" virDomainObjListFindByNameLocked >> returns NULL (although the definition actually exists). Therefore, the >> possibility exits that "virHashAddEntry" will raise the error >> "Duplicate key" => virDomainObjListAddObjLocked fails => >> virDomainObjEndAPI(&vm) is called and this leads to a freeing of @def >> since @def is already assigned to vm->def. But actually this leads to >> a double free since the common usage pattern is that the caller of >> virDomainObjListAdd(Locked) is responsible for freeing @def in case of >> an error. >> >> Let's fix this by setting vm->def to NULL in case of an error. >> >> Backtrace: >> >> ➤ bt >> #0 virFree (ptrptr=0x7575757575757575) >> #1 0x000003ffb5b25b3e in virDomainResourceDefFree >> #2 0x000003ffb5b37c34 in virDomainDefFree >> #3 0x000003ff9123f734 in qemuDomainDefineXMLFlags >> #4 0x000003ff9123f7f4 in qemuDomainDefineXML >> #5 0x000003ffb5cd2c84 in virDomainDefineXML >> #6 0x000000011745aa82 in remoteDispatchDomainDefineXML >> ... >> >> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> --- >> src/conf/virdomainobjlist.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c >> index 52171594f34f..805fe9440afe 100644 >> --- a/src/conf/virdomainobjlist.c >> +++ b/src/conf/virdomainobjlist.c >> @@ -329,8 +329,10 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, >> goto cleanup; >> vm->def = def; >> >> - if (virDomainObjListAddObjLocked(doms, vm) < 0) >> + if (virDomainObjListAddObjLocked(doms, vm) < 0) { >> + vm->def = NULL; >> goto error; >> + } >> } >> cleanup: >> return vm; >> > > > How about setting vm->def only after virDomainObjListAddObjLocked() > succeded? Unfortunately, this doesn’t work as vm->def->name is used by virDomainObjListAddObjLocked(). Another solution that came to my mind is to “hand over” the responsibility for @def to virDomainObjListAdd(Locked) (this would require that a 'virDomainDefPtr *def' instead of 'virDomainDefPtr def' is passed). This would also eliminates the “def = NULL;” lines after a successful virDomainObjListAddObj call and it would solve this bug. > > However, this points to a more sever bug. If a domain is being removed, > and some other thread is trying to define it back, I guess the whole > operation should succeed. Definitely not fail with some (for user) > cryptic message like "double key found in a hash table". > > The problem is that: > > a) both virDomainObjListFindByUUIDLocked() and > virDomainObjListFindByNameLocked() return NULL even if domain exists. > They should return the found domain and only the caller should decide if > vm->removing is relevant or not. Yep, but this is another problem since virDomainObjListAddObjLocked can also fail under other circumstances. > > b) nothing is clearing out the vm->removing flag. The > virDomainObjListAddObjLocked() looks like a good candidate to do so. Haven’t looked into much detail for that part. > > Michal > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 08/27/2018 06:06 PM, Marc Hartmayer wrote: > On Mon, Aug 27, 2018 at 04:03 PM +0200, Michal Prívozník <mprivozn@redhat.com> wrote: >> On 08/27/2018 03:20 PM, Marc Hartmayer wrote: >>> If @vm has flagged as "to be removed" virDomainObjListFindByNameLocked >>> returns NULL (although the definition actually exists). Therefore, the >>> possibility exits that "virHashAddEntry" will raise the error >>> "Duplicate key" => virDomainObjListAddObjLocked fails => >>> virDomainObjEndAPI(&vm) is called and this leads to a freeing of @def >>> since @def is already assigned to vm->def. But actually this leads to >>> a double free since the common usage pattern is that the caller of >>> virDomainObjListAdd(Locked) is responsible for freeing @def in case of >>> an error. >>> >>> Let's fix this by setting vm->def to NULL in case of an error. >>> >>> Backtrace: >>> >>> ➤ bt >>> #0 virFree (ptrptr=0x7575757575757575) >>> #1 0x000003ffb5b25b3e in virDomainResourceDefFree >>> #2 0x000003ffb5b37c34 in virDomainDefFree >>> #3 0x000003ff9123f734 in qemuDomainDefineXMLFlags >>> #4 0x000003ff9123f7f4 in qemuDomainDefineXML >>> #5 0x000003ffb5cd2c84 in virDomainDefineXML >>> #6 0x000000011745aa82 in remoteDispatchDomainDefineXML >>> ... >>> >>> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> >>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> >>> --- >>> src/conf/virdomainobjlist.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c >>> index 52171594f34f..805fe9440afe 100644 >>> --- a/src/conf/virdomainobjlist.c >>> +++ b/src/conf/virdomainobjlist.c >>> @@ -329,8 +329,10 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, >>> goto cleanup; >>> vm->def = def; >>> >>> - if (virDomainObjListAddObjLocked(doms, vm) < 0) >>> + if (virDomainObjListAddObjLocked(doms, vm) < 0) { >>> + vm->def = NULL; >>> goto error; >>> + } >>> } >>> cleanup: >>> return vm; >>> >> >> >> How about setting vm->def only after virDomainObjListAddObjLocked() >> succeded? > > Unfortunately, this doesn’t work as vm->def->name is used by > virDomainObjListAddObjLocked(). Ah right. > > Another solution that came to my mind is to “hand over” the > responsibility for @def to virDomainObjListAdd(Locked) (this would > require that a 'virDomainDefPtr *def' instead of 'virDomainDefPtr def' > is passed). This would also eliminates the “def = NULL;” lines after a > successful virDomainObjListAddObj call and it would solve this bug. Yep, this looks like a good solution. Because we are close to the release I am going to push this version and what you are suggesting can then be saved to a follow up patch. > >> >> However, this points to a more sever bug. If a domain is being removed, >> and some other thread is trying to define it back, I guess the whole >> operation should succeed. Definitely not fail with some (for user) >> cryptic message like "double key found in a hash table". >> >> The problem is that: >> >> a) both virDomainObjListFindByUUIDLocked() and >> virDomainObjListFindByNameLocked() return NULL even if domain exists. >> They should return the found domain and only the caller should decide if >> vm->removing is relevant or not. > > Yep, but this is another problem since virDomainObjListAddObjLocked can > also fail under other circumstances. > >> >> b) nothing is clearing out the vm->removing flag. The >> virDomainObjListAddObjLocked() looks like a good candidate to do so. > > Haven’t looked into much detail for that part. Okay, I'll look into it. By skimming the code quickly I can see a potential problem there. But maybe I'm wrong. ACKed and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Michal Prívozník <mprivozn@redhat.com> [2018-08-27, 04:03PM +0200]: > On 08/27/2018 03:20 PM, Marc Hartmayer wrote: > > If @vm has flagged as "to be removed" virDomainObjListFindByNameLocked > > returns NULL (although the definition actually exists). Therefore, the > > possibility exits that "virHashAddEntry" will raise the error > > "Duplicate key" => virDomainObjListAddObjLocked fails => > > virDomainObjEndAPI(&vm) is called and this leads to a freeing of @def > > since @def is already assigned to vm->def. But actually this leads to > > a double free since the common usage pattern is that the caller of > > virDomainObjListAdd(Locked) is responsible for freeing @def in case of > > an error. > > > > Let's fix this by setting vm->def to NULL in case of an error. > > > > Backtrace: > > > > ➤ bt > > #0 virFree (ptrptr=0x7575757575757575) > > #1 0x000003ffb5b25b3e in virDomainResourceDefFree > > #2 0x000003ffb5b37c34 in virDomainDefFree > > #3 0x000003ff9123f734 in qemuDomainDefineXMLFlags > > #4 0x000003ff9123f7f4 in qemuDomainDefineXML > > #5 0x000003ffb5cd2c84 in virDomainDefineXML > > #6 0x000000011745aa82 in remoteDispatchDomainDefineXML > > ... > > > > Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> > > Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> > > --- > > src/conf/virdomainobjlist.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c > > index 52171594f34f..805fe9440afe 100644 > > --- a/src/conf/virdomainobjlist.c > > +++ b/src/conf/virdomainobjlist.c > > @@ -329,8 +329,10 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, > > goto cleanup; > > vm->def = def; > > > > - if (virDomainObjListAddObjLocked(doms, vm) < 0) > > + if (virDomainObjListAddObjLocked(doms, vm) < 0) { > > + vm->def = NULL; > > goto error; > > + } > > } > > cleanup: > > return vm; > > > > > How about setting vm->def only after virDomainObjListAddObjLocked() > succeded? > > However, this points to a more sever bug. If a domain is being removed, > and some other thread is trying to define it back, I guess the whole > operation should succeed. Definitely not fail with some (for user) > cryptic message like "double key found in a hash table". > > The problem is that: > > a) both virDomainObjListFindByUUIDLocked() and > virDomainObjListFindByNameLocked() return NULL even if domain exists. > They should return the found domain and only the caller should decide if > vm->removing is relevant or not. > > b) nothing is clearing out the vm->removing flag. The > virDomainObjListAddObjLocked() looks like a good candidate to do so. > > Michal Can we still take this fix for the upcoming release and worry about doing it right later? > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- IBM Systems Linux on Z & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 ------------------------------------------------------------------------ Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.