[PATCH] virt-aa-helper: Fix unchecked return values
Add missing return value checks to fix the following issues reported by the static analyzer: - vah_add_file() call when adding render node path to the AppArmor profile (line 1029) was not checked, while there are examples with return code check throughout the code. - vah_add_file() call when adding default render node path (line 1037) had the same issue. - virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code. Signed-off-by: Dmitry Lopatin <dmitry.lopatin@flant.com> --- src/security/virt-aa-helper.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 067a17f331..07e5882237 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -905,7 +905,8 @@ get_files(vahControl * ctl) /* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + goto cleanup; #endif for (i = 0; i < ctl->def->ndisks; i++) { @@ -1026,7 +1027,8 @@ get_files(vahControl * ctl) const char *rendernode = virDomainGraphicsGetRenderNode(graphics); if (rendernode) { - vah_add_file(&buf, rendernode, "rw"); + if (vah_add_file(&buf, rendernode, "rw") != 0) + goto cleanup; needsgl = true; } else { if (virDomainGraphicsNeedsAutoRenderNode(graphics)) { @@ -1034,7 +1036,8 @@ get_files(vahControl * ctl) needsgl = true; if (defaultRenderNode) { - vah_add_file(&buf, defaultRenderNode, "rw"); + if (vah_add_file(&buf, defaultRenderNode, "rw") != 0) + goto cleanup; VIR_FREE(defaultRenderNode); } } -- 2.34.1
Sorry for the ping, I worry this patch was missed, because its my first contribution and I've sent patch without subscription and it was delayed.
Sorry for the ping, I worry this patch was missed, because its my first contribution and I've sent patch without subscription and it was delayed. 21.01.2026 10:41, Dmitry Lopatin пишет:
Add missing return value checks to fix the following issues reported by the static analyzer:
- vah_add_file() call when adding render node path to the AppArmor profile (line 1029) was not checked, while there are examples with return code check throughout the code.
- vah_add_file() call when adding default render node path (line 1037) had the same issue.
- virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code.
Signed-off-by: Dmitry Lopatin <dmitry.lopatin@flant.com> --- src/security/virt-aa-helper.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 067a17f331..07e5882237 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -905,7 +905,8 @@ get_files(vahControl * ctl)
/* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + goto cleanup; #endif
for (i = 0; i < ctl->def->ndisks; i++) { @@ -1026,7 +1027,8 @@ get_files(vahControl * ctl) const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
if (rendernode) { - vah_add_file(&buf, rendernode, "rw"); + if (vah_add_file(&buf, rendernode, "rw") != 0) + goto cleanup; needsgl = true; } else { if (virDomainGraphicsNeedsAutoRenderNode(graphics)) { @@ -1034,7 +1036,8 @@ get_files(vahControl * ctl) needsgl = true;
if (defaultRenderNode) { - vah_add_file(&buf, defaultRenderNode, "rw"); + if (vah_add_file(&buf, defaultRenderNode, "rw") != 0) + goto cleanup; VIR_FREE(defaultRenderNode); } }
On a Thursday in 2026, dmitry.lopatin@flant.com wrote:
Sorry for the ping, I worry this patch was missed, because its my first contribution and I've sent patch without subscription and it was delayed.
Yeah, the setup is a bit unfortunate. You can sometimes check in the archives if it made it through: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/ But there have also been situations when just the archives were broken. On a Wednesday in 2026, Dmitry Lopatin wrote:
Add missing return value checks to fix the following issues reported by the static analyzer:
- vah_add_file() call when adding render node path to the AppArmor profile (line 1029) was not checked, while there are examples with return code check throughout the code.
- vah_add_file() call when adding default render node path (line 1037) had the same issue.
- virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code.
Signed-off-by: Dmitry Lopatin <dmitry.lopatin@flant.com> --- src/security/virt-aa-helper.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
The patch does not apply for me on current master, please send patches against the current master branch.
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 067a17f331..07e5882237 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -905,7 +905,8 @@ get_files(vahControl * ctl)
/* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + goto cleanup; #endif
for (i = 0; i < ctl->def->ndisks; i++) {
@@ -1026,7 +1027,8 @@ get_files(vahControl * ctl) const char *rendernode = virDomainGraphicsGetRenderNode(graphics);
if (rendernode) { - vah_add_file(&buf, rendernode, "rw"); + if (vah_add_file(&buf, rendernode, "rw") != 0) + goto cleanup; needsgl = true; } else { if (virDomainGraphicsNeedsAutoRenderNode(graphics)) { @@ -1034,7 +1036,8 @@ get_files(vahControl * ctl) needsgl = true;
if (defaultRenderNode) { - vah_add_file(&buf, defaultRenderNode, "rw"); + if (vah_add_file(&buf, defaultRenderNode, "rw") != 0) + goto cleanup; VIR_FREE(defaultRenderNode); } }
These two vah_add_file calls are already checked since: commit ecca0dded412c84c3c89f9e4f1d6f2c5c57b4174 Author: Michal Prívozník <mprivozn@redhat.com> AuthorDate: 2025-06-11 13:59:49 +0200 Commit: Michal Prívozník <mprivozn@redhat.com> CommitDate: 2025-07-02 13:54:30 +0200 virt-aa-helper: Check retval of vah_add_file() Which was already released in libvirt 11.6.0 Jano
- virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code. --- src/security/virt-aa-helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index f4ec6b7826..2de1b31b5a 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -892,7 +892,8 @@ get_files(vahControl * ctl) /* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + goto cleanup;("storage", "storageRegister", false); #endif for (i = 0; i < ctl->def->ndisks; i++) { -- 2.34.1
Add missing return value checks to fix the following issues reported by the static analyzer: - virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code. --- src/security/virt-aa-helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index f4ec6b7826..2de1b31b5a 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -892,7 +892,8 @@ get_files(vahControl * ctl) /* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + goto cleanup;("storage", "storageRegister", false); #endif for (i = 0; i < ctl->def->ndisks; i++) { -- 2.34.1
On Wed, Feb 04, 2026 at 22:29:16 +0300, Dmitry Lopatin wrote:
Add missing return value checks to fix the following issues reported by the static analyzer:
- virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code.
Your patch is missing declaration that it conforms to the Developer Certificate of Origin, which is required for all our submissions: https://www.libvirt.org/hacking.html#developer-certificate-of-origin Although now I see that your v1 had this.
--- src/security/virt-aa-helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index f4ec6b7826..2de1b31b5a 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -892,7 +892,8 @@ get_files(vahControl * ctl)
/* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + goto cleanup;("storage", "storageRegister", false);
^^^^^^^^^^ This looks very broken
#endif
for (i = 0; i < ctl->def->ndisks; i++) { -- 2.34.1
Add missing return value checks to fix the following issues reported by the static analyzer: - virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code. Signed-off-by: Dmitry Lopatin <dmitry.lopatin@flant.com> --- src/security/virt-aa-helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index f4ec6b7826..624da61a0f 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -892,7 +892,8 @@ get_files(vahControl * ctl) /* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + goto cleanup; #endif for (i = 0; i < ctl->def->ndisks; i++) { -- 2.34.1
On Fri, Feb 06, 2026 at 13:03:04 +0300, Dmitry Lopatin wrote:
Add missing return value checks to fix the following issues reported by the static analyzer:
- virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code.
Signed-off-by: Dmitry Lopatin <dmitry.lopatin@flant.com> --- src/security/virt-aa-helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index f4ec6b7826..624da61a0f 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -892,7 +892,8 @@ get_files(vahControl * ctl)
/* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + goto cleanup;
There's no 'cleanup' label in this function: FAILED: src/virt-aa-helper.p/security_virt-aa-helper.c.o ccache cc -Isrc/virt-aa-helper.p -Isrc -I../src -Isrc/conf -I../src/conf -Isrc/hypervisor -I../src/hypervisor -Isrc/security -I../src/security -Isrc/storage_file -I../src/storage_file -Isrc/util -I../src/util -Iinclude -I../include -I. -I.. -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/gio-unix-2.0 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/libxml2 -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu99 -O2 -g @/builds/pipo.sk/libvirt/build/c-warnings.txt -fPIE -pthread -MD -MQ src/virt-aa-helper.p/security_virt-aa-helper.c.o -MF src/virt-aa-helper.p/security_virt-aa-helper.c.o.d -o src/virt-aa-helper.p/security_virt-aa-helper.c.o -c ../src/security/virt-aa-helper.c ../src/security/virt-aa-helper.c: In function ‘get_files’: ../src/security/virt-aa-helper.c:896:9: error: label ‘cleanup’ used but not defined 896 | goto cleanup; | ^~~~ https://gitlab.com/pipo.sk/libvirt/-/jobs/13015050448#L1767
Add missing return value checks to fix the following issues reported by the static analyzer: - virDriverLoadModule() call when loading the storage driver (line 908) was not checked, while there are examples with return code check throughout the code. Signed-off-by: Dmitry Lopatin <dmitry.lopatin@flant.com> --- src/security/virt-aa-helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index f4ec6b7826..38625f3721 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -892,7 +892,8 @@ get_files(vahControl * ctl) /* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister", false); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + return -1; #endif for (i = 0; i < ctl->def->ndisks; i++) { -- 2.34.1
participants (5)
-
Dmitry Lopatin -
dmitry lopatin -
dmitry.lopatin＠flant.com
-
Ján Tomko -
Peter Krempa