[libvirt] [PATCH] util: Allow removing hash entries in virHashForEach
This fixes a possible crash of libvirtd during its startup. When qemu driver reconnects to running domains, it iterates over all domain objects in a hash. When reconnecting to an associated qemu monitor fails and the domain is transient, it's immediately removed from the hash. Despite the fact that it's explicitly forbidden to do so. If libvirtd is lucky enough, virHashForEach will access random memory when the callback finishes and the deamon will crash. Since it's trivial to fix virHashForEach to allow removal of hash entries while iterating through them, I went this way instead of fixing qemuReconnectDomain callback (and possibly others) to avoid deleting the entries. --- src/util/hash.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index 92ee234..2a9a9cf 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -548,9 +548,8 @@ virHashRemoveEntry(virHashTablePtr table, const void *name) * @data: opaque data to pass to the iterator * * Iterates over every element in the hash table, invoking the - * 'iter' callback. The callback must not call any other virHash* - * functions, and in particular must not attempt to remove the - * element. + * 'iter' callback. The callback is allowed to remove the element using + * virHashRemoveEntry but calling other virHash* functions is prohibited. * * Returns number of items iterated over upon completion, -1 on failure */ @@ -563,11 +562,12 @@ int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) { for (i = 0 ; i < table->size ; i++) { virHashEntryPtr entry = table->table + i; while (entry) { + virHashEntryPtr next = entry->next; if (entry->valid) { iter(entry->payload, entry->name, data); count++; } - entry = entry->next; + entry = next; } } return (count); -- 1.7.4.1
On Thu, Mar 03, 2011 at 02:46:57PM +0100, Jiri Denemark wrote:
This fixes a possible crash of libvirtd during its startup. When qemu driver reconnects to running domains, it iterates over all domain objects in a hash. When reconnecting to an associated qemu monitor fails and the domain is transient, it's immediately removed from the hash. Despite the fact that it's explicitly forbidden to do so. If libvirtd is lucky enough, virHashForEach will access random memory when the callback finishes and the deamon will crash.
Since it's trivial to fix virHashForEach to allow removal of hash entries while iterating through them, I went this way instead of fixing qemuReconnectDomain callback (and possibly others) to avoid deleting the entries. --- src/util/hash.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/hash.c b/src/util/hash.c index 92ee234..2a9a9cf 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -548,9 +548,8 @@ virHashRemoveEntry(virHashTablePtr table, const void *name) * @data: opaque data to pass to the iterator * * Iterates over every element in the hash table, invoking the - * 'iter' callback. The callback must not call any other virHash* - * functions, and in particular must not attempt to remove the - * element. + * 'iter' callback. The callback is allowed to remove the element using + * virHashRemoveEntry but calling other virHash* functions is prohibited. * * Returns number of items iterated over upon completion, -1 on failure */ @@ -563,11 +562,12 @@ int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) { for (i = 0 ; i < table->size ; i++) { virHashEntryPtr entry = table->table + i; while (entry) { + virHashEntryPtr next = entry->next; if (entry->valid) { iter(entry->payload, entry->name, data); count++; } - entry = entry->next; + entry = next; } } return (count);
ACK, removing should be safe then, but adding will still be a big problem due to virHashGrow(). It's a safety belt, but doesn't replace driving properly. I tend to think we should try to detect reentrant behaviour, for example have a counter increment each time an operation is done on the hash and raise a warning at runtime if the counter changed around iter(). Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Mar 03, 2011 at 22:09:32 +0800, Daniel Veillard wrote:
This fixes a possible crash of libvirtd during its startup. When qemu driver reconnects to running domains, it iterates over all domain objects in a hash. When reconnecting to an associated qemu monitor fails and the domain is transient, it's immediately removed from the hash. Despite the fact that it's explicitly forbidden to do so. If libvirtd is lucky enough, virHashForEach will access random memory when the callback finishes and the deamon will crash.
Since it's trivial to fix virHashForEach to allow removal of hash entries while iterating through them, I went this way instead of fixing qemuReconnectDomain callback (and possibly others) to avoid deleting the entries.
ACK, removing should be safe then, but adding will still be a big problem due to virHashGrow().
Yeah, that would be a much bigger issue. I pushed the patch.
It's a safety belt, but doesn't replace driving properly. I tend to think we should try to detect reentrant behaviour, for example have a counter increment each time an operation is done on the hash and raise a warning at runtime if the counter changed around iter().
It would probably be easier to set a flag that we're iterating over the hash and let all other APIs fail in that case. Jirka
On Thu, Mar 03, 2011 at 03:46:05PM +0100, Jiri Denemark wrote:
On Thu, Mar 03, 2011 at 22:09:32 +0800, Daniel Veillard wrote:
This fixes a possible crash of libvirtd during its startup. When qemu driver reconnects to running domains, it iterates over all domain objects in a hash. When reconnecting to an associated qemu monitor fails and the domain is transient, it's immediately removed from the hash. Despite the fact that it's explicitly forbidden to do so. If libvirtd is lucky enough, virHashForEach will access random memory when the callback finishes and the deamon will crash.
Since it's trivial to fix virHashForEach to allow removal of hash entries while iterating through them, I went this way instead of fixing qemuReconnectDomain callback (and possibly others) to avoid deleting the entries.
ACK, removing should be safe then, but adding will still be a big problem due to virHashGrow().
Yeah, that would be a much bigger issue.
I pushed the patch.
It's a safety belt, but doesn't replace driving properly. I tend to think we should try to detect reentrant behaviour, for example have a counter increment each time an operation is done on the hash and raise a warning at runtime if the counter changed around iter().
It would probably be easier to set a flag that we're iterating over the hash and let all other APIs fail in that case.
Works too :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/03/2011 03:09 PM, Daniel Veillard wrote:
ACK, removing should be safe then, but adding will still be a big problem due to virHashGrow().
Removing _the current_ element is safe, on the other hand removing any other element used to be safe and now it may cause a dangling pointer access... Paolo
participants (3)
-
Daniel Veillard -
Jiri Denemark -
Paolo Bonzini