[PATCH] virt-login-shell: check for NULL from virGetLastError()
From: Denis Rastyogin <gerben@altlinux.org> virGetLastError() may return NULL in case of OOM. Without a check this could lead to a NULL pointer dereference when accessing its fields. The result of virGetLastError() is usually checked in other places, so add the missing check here as well. Found by Linux Verification Center (linuxtesting.org) with SVACE. Reported-by: Pavel Nekrasov <p.nekrasov@fobos-nt.ru> Signed-off-by: Denis Rastyogin <gerben@altlinux.org> --- tools/virt-login-shell-helper.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/virt-login-shell-helper.c b/tools/virt-login-shell-helper.c index cb59b5dec0..9282ca481e 100644 --- a/tools/virt-login-shell-helper.c +++ b/tools/virt-login-shell-helper.c @@ -282,6 +282,10 @@ main(int argc, char **argv) if (!virDomainIsActive(dom) && virDomainCreate(dom) < 0) { virErrorPtr last_error; last_error = virGetLastError(); + + if (!last_error) + goto cleanup; + if (last_error->code != VIR_ERR_OPERATION_INVALID) { virReportSystemError(last_error->code, _("Can't create %1$s container: %2$s"), -- 2.42.2
On Mon, Sep 29, 2025 at 07:55:55PM +0300, gerben@altlinux.org wrote:
From: Denis Rastyogin <gerben@altlinux.org>
virGetLastError() may return NULL in case of OOM. Without a check this could lead to a NULL pointer dereference when accessing its fields. The result of virGetLastError() is usually checked in other places, so add the missing check here as well.
AFAICT, it can't return NULL on OOM, because virGetLastError calls into virLastErrorObject() which calls g_new0() which aborts on OOM. The only way we can get NULL from virGetLastError is if there was no error reported, because in this codepath we've already seen that virDomainCreate returned < 0, and so we know an error is reported. IOW, I don't see any bug here to fix.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Reported-by: Pavel Nekrasov <p.nekrasov@fobos-nt.ru> Signed-off-by: Denis Rastyogin <gerben@altlinux.org> --- tools/virt-login-shell-helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/virt-login-shell-helper.c b/tools/virt-login-shell-helper.c index cb59b5dec0..9282ca481e 100644 --- a/tools/virt-login-shell-helper.c +++ b/tools/virt-login-shell-helper.c @@ -282,6 +282,10 @@ main(int argc, char **argv) if (!virDomainIsActive(dom) && virDomainCreate(dom) < 0) { virErrorPtr last_error; last_error = virGetLastError(); + + if (!last_error) + goto cleanup; + if (last_error->code != VIR_ERR_OPERATION_INVALID) { virReportSystemError(last_error->code, _("Can't create %1$s container: %2$s"), -- 2.42.2
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Indeed, when virDomainCreate() < 0, virDispatchError() should be invoked, but since we do not check its return value, there is no absolute guarantee that the error message will always be set successfully. virDispatchError() contains the following code: virErrorPtr err = virLastErrorObject(); /* Can only happen on OOM. */ if (!err) return; The comment clearly indicates that virLastErrorObject() may return NULL in case of OOM. Moreover, even if the call err = g_new0(virError, 1); succeeds, this does not guarantee that virThreadLocalSet(&virLastErr, err) < 0 will always false. For example, for large values of l->key, an additional memory allocation may occur, and if that allocation fails, g_clear_pointer(&err, g_free) will be executed, causing virLastErrorObject() to return NULL. Although this situation is extremely unlikely, it is better to add an explicit NULL check.
On Wed, Oct 01, 2025 at 02:25:55PM -0000, Denis Rastyogin wrote:
Indeed, when virDomainCreate() < 0, virDispatchError() should be invoked, but since we do not check its return value, there is no absolute guarantee that the error message will always be set successfully.
virDispatchError() contains the following code:
virErrorPtr err = virLastErrorObject();
/* Can only happen on OOM. */ if (!err) return;
The comment clearly indicates that virLastErrorObject() may return NULL in case of OOM.
That's just an outdated comment from before we switched all allocations to g_new0 to eliminate OOM handling.
Moreover, even if the call err = g_new0(virError, 1); succeeds, this does not guarantee that virThreadLocalSet(&virLastErr, err) < 0 will always false.
For example, for large values of l->key, an additional memory allocation may occur, and if that allocation fails, g_clear_pointer(&err, g_free) will be executed, causing virLastErrorObject() to return NULL.
Although this situation is extremely unlikely, it is better to add an explicit NULL check.
No, it really isn't useful to do that. If pthread_setspecific is failing with OOM then there is nothing useful the program can still do. The only sane thing is to assert & dump core. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
Denis Rastyogin -
gerben＠altlinux.org