[libvirt] [PATCH] virDomainObjListAddLocked: fix double free
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
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
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
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
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
participants (4)
-
Bjoern Walk -
Marc Hartmayer -
Michal Privoznik -
Michal Prívozník