On a Thursday in 2025, Michal Privoznik via Devel wrote: That's a good enough reason to forbid it in syntax-check. In fact, the use of realpath in virFilleCanonicalizePath already has a comment it's exempt from syntax-check. There is no rule for it, because the comment is a leftover from when it used canonicalize_file_name: commit b4d601ba87ade7fa1a3a4f9c0c268659c15a35c3 util: use realpath/g_canonicalize_filename The syntax-check rule was specifically introduced because of mocking: commit 00d465bb4df4a49e07d51ce289142b5fae8ba344 syntax-check: Prohibit canonicalize_file_name() Jano

From: Michal Privoznik <mprivozn(a)redhat.com&gt; While use of realpath() is not forbidden, our some of our mocks already have a test friendly reimplementation of virFileCanonicalizePath(). Use the latter. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/virt-aa-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e3802c18be..d4358ebf9c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -712,7 +712,7 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive tmp = g_strdup(path); } else { pathtmp = g_strdup(path + strlen(pathdir)); - if ((pathreal = realpath(pathdir, NULL)) == NULL) { + if (!(pathreal = virFileCanonicalizePath(pathdir))) { vah_error(NULL, 0, pathdir); vah_error(NULL, 0, _("could not find realpath")); return rc; -- 2.49.0

From: Michal Privoznik <mprivozn(a)redhat.com&gt; The mocked implementation of virFileCanonicalizePath() redirects accesses to few dirs into a temporary directory, where PCI related files live. See getrealpath() for more info on this. Anyway, in the end - real implementation of virFileCanonicalizePath() is called which then might contain the 'fakerootdir' prefix. Up until now this did not matter because none of our test really cared about actual value of resolved path. They usually cared about last component of the path or something. But this will soon change. TLDR - if the returned path has $fakerootdir prefix, strip it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/virpcimock.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 34128d5516..4eff6d70e3 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -1184,13 +1184,23 @@ char * virFileCanonicalizePath(const char *path) { g_autofree char *newpath = NULL; + char *ret = NULL; init_syms(); if (getrealpath(&newpath, path) < 0) return NULL; - return real_virFileCanonicalizePath(newpath); + ret = real_virFileCanonicalizePath(newpath); + + if (ret && fakerootdir && STRPREFIX(ret, fakerootdir)) { + size_t len = strlen(ret); + size_t preflen = strlen(fakerootdir); + + memmove(ret, ret + preflen, len - preflen + 1); + } + + return ret; } # include "virmockstathelpers.c" -- 2.49.0

From: Michal Privoznik <mprivozn(a)redhat.com&gt; When a test case fails, there are two echo-s executed: the first one either prints the error message into /dev/null (default) or onto stdout (when the test script is executed with -d). Then, the second one prints the error message onto stdout. While this technically works, there's nothing ever printed onto stderr which is usually what's captured. Worse, if some command within the script fails, it prints something onto stderr but then looking at meson logs it's needlessly hard to match stderr and stdout lines. Just print error messages onto stderr. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/virt-aa-helper-test | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 4c8d31c9d7..8259c2679f 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -116,7 +116,7 @@ testme() { input="$4" if [ ! -e "$input" ]; then echo "FAIL: could not find $input" >$output - echo "FAIL: could not find $input" + echo "FAIL: could not find $input" >&2 echo " '$extra_args $args': " errors=$(($errors + 1)) fi @@ -141,6 +141,7 @@ testme() { if [ -n "$checkrule" ]; then if ! grep "$checkrule" "$tmpout" >/dev/null; then echo "FAIL: missing rule '$checkrule'" >"$output" + echo "FAIL: missing rule '$checkrule'" >&2 rule_missing=1 fi fi -- 2.49.0

From: Michal Privoznik <mprivozn(a)redhat.com&gt; Our test suite is very feature rich. In particular, it has two mocks that implement sysfs close enough to create host-independent environment to work with PCI and USB devices. These mocks are called virpcimock and virusbmock, respectively. Inside of virt-aa-helper-test there is an attempt to test whether virt-aa-helper generates profiles for <hostdevs/>, once for USB and the other time for PCI. Use this mocks to run virt-aa-helper in an environment where certain PCI/USB devices always exist. There are two problem though: 1) those two test cases use hardcoded PCI/USB addresses, which makes them host environment dependant, 2) neither of the test cases checks whether corresponding rule was added into the profile. Using mocks we can get away with problem 1), and by passing the fifth argument to testme() we can list an expected rule in the profile. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/virt-aa-helper-test | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index e462e46570..c0b8c1bafe 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -7,15 +7,9 @@ set -e -test_hostdev="no" -if [ "$1" = "test_hostdev" ]; then - test_hostdev="yes" - shift -fi - output="/dev/null" use_valgrind="" -ld_library_path="$abs_top_builddir/src/" +ld_library_path="$abs_top_builddir/tests/:$abs_top_builddir/src/" if [ ! -z "$1" ] && [ "$1" = "-d" ]; then output="/dev/stdout" shift @@ -128,11 +122,12 @@ testme() { printf %s " < $input" >$output fi echo "': " >$output + ld_preload="libvirusbmock.so:libvirpcimock.so" set +e if [ -n "$input" ]; then - LD_LIBRARY_PATH="$ld_library_path" "${exe}" $extra_args $args < $input >"$tmpout" 2>&1 + LD_PRELOAD="$ld_preload" LD_LIBRARY_PATH="$ld_library_path" ${exe} $extra_args $args < $input >"$tmpout" 2>&1 else - LD_LIBRARY_PATH="$ld_library_path" "${exe}" $extra_args $args >"$tmpout" 2>&1 + LD_PRELOAD="$ld_preload" LD_LIBRARY_PATH="$ld_library_path" ${exe} $extra_args $args >"$tmpout" 2>&1 fi rc="$?" cat "$tmpout" >"$output" @@ -262,13 +257,11 @@ testme "0" "create multiple disks" "-c -u $valid_uuid" "$test_xml" "$disk1.*rwk, sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###',${disk1}'/><readonly,g" "$template_xml" > "$test_xml" testme "0" "create (readonly)" "-c -u $valid_uuid" "$test_xml" "$disk1.*rk,$" -if [ "$test_hostdev" = "yes" ]; then - sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</disk>,</disk><hostdev mode='subsystem' type='usb'><source><address bus='002' device='004'/></source></hostdev>,g" "$template_xml" > "$test_xml" - testme "0" "create hostdev (USB)" "-c -u $valid_uuid" "$test_xml" +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</disk>,</disk><hostdev mode='subsystem' type='usb'><source><address bus='001' device='024'/></source></hostdev>,g" "$template_xml" > "$test_xml" +testme "0" "create hostdev (USB)" "-c -u $valid_uuid" "$test_xml" "/dev/bus/usb/001/020" - sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</disk>,</disk><hostdev mode='subsystem' type='pci'><source><address bus='0x00' slot='0x19' function='0x0'/></source></hostdev>,g" "$template_xml" > "$test_xml" - testme "0" "create hostdev (PCI)" "-c -u $valid_uuid" "$test_xml" -fi +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</disk>,</disk><hostdev mode='subsystem' type='pci'><source><address bus='0x00' slot='0x3' function='0x0'/></source></hostdev>,g" "$template_xml" > "$test_xml" +testme "0" "create hostdev (PCI)" "-c -u $valid_uuid" "$test_xml" "/sys/devices/pci0000:00/0000:00:03.0/config" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$nonexistent,g" "$template_xml" > "$test_xml" testme "0" "create (non-existent disk)" "-c -u $valid_uuid" "$test_xml" "$nonexistent.*rwk,$" -- 2.49.0

From: Michal Privoznik <mprivozn(a)redhat.com&gt; The way virt-aa-helper works is the following: the apparmor secdriver formats domain XML, spawns virt-aa-helper process and feeds it with domain XML (through stdin). The helper process then parses the XML and iterates over devices, appending paths in each loop. These loops usually are in the following form: for (i = 0; i < ctl->def->nserials; i++) { if (ctl->def->serials[i] && ... } While we are probably honourable members of tautology club, those NULL checks are redundant. Our XML parses would never append NULL into def->devices array. If it did, we're in way bigger problems anyway. Then, constantly dereferencing ctl->def just to get to a path that's hidden a couple of structures deep gets hard to read. Just introduce temporary variables. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/virt-aa-helper.c | 329 ++++++++++++++++++---------------- 1 file changed, 173 insertions(+), 156 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a56d7e9062..2fac65f108 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -904,63 +904,79 @@ get_files(vahControl * ctl) goto cleanup; } - for (i = 0; i < ctl->def->nserials; i++) - if (ctl->def->serials[i] && - (ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || - ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || - ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || - ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || - ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PIPE) && - ctl->def->serials[i]->source->data.file.path && - ctl->def->serials[i]->source->data.file.path[0] != '\0') + for (i = 0; i < ctl->def->nserials; i++) { + virDomainChrDef *chr = ctl->def->serials[i]; + + if ((chr->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + chr->source->type == VIR_DOMAIN_CHR_TYPE_DEV || + chr->source->type == VIR_DOMAIN_CHR_TYPE_FILE || + chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || + chr->source->type == VIR_DOMAIN_CHR_TYPE_PIPE) && + chr->source->data.file.path && + chr->source->data.file.path[0] != '\0') { if (vah_add_file_chardev(&buf, - ctl->def->serials[i]->source->data.file.path, + chr->source->data.file.path, "rw", - ctl->def->serials[i]->source->type) != 0) + chr->source->type) != 0) { goto cleanup; + } + } + } - for (i = 0; i < ctl->def->nconsoles; i++) - if (ctl->def->consoles[i] && - (ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || - ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || - ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || - ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || - ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PIPE) && - ctl->def->consoles[i]->source->data.file.path && - ctl->def->consoles[i]->source->data.file.path[0] != '\0') + for (i = 0; i < ctl->def->nconsoles; i++) { + virDomainChrDef *chr = ctl->def->consoles[i]; + + if ((chr->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + chr->source->type == VIR_DOMAIN_CHR_TYPE_DEV || + chr->source->type == VIR_DOMAIN_CHR_TYPE_FILE || + chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || + chr->source->type == VIR_DOMAIN_CHR_TYPE_PIPE) && + chr->source->data.file.path && + chr->source->data.file.path[0] != '\0') { if (vah_add_file(&buf, - ctl->def->consoles[i]->source->data.file.path, "rw") != 0) + chr->source->data.file.path, "rw") != 0) { goto cleanup; + } + } + } - for (i = 0; i < ctl->def->nparallels; i++) - if (ctl->def->parallels[i] && - (ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || - ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || - ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || - ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || - ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PIPE) && - ctl->def->parallels[i]->source->data.file.path && - ctl->def->parallels[i]->source->data.file.path[0] != '\0') + for (i = 0; i < ctl->def->nparallels; i++) { + virDomainChrDef *chr = ctl->def->parallels[i]; + + if ((chr->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + chr->source->type == VIR_DOMAIN_CHR_TYPE_DEV || + chr->source->type == VIR_DOMAIN_CHR_TYPE_FILE || + chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || + chr->source->type == VIR_DOMAIN_CHR_TYPE_PIPE) && + chr->source->data.file.path && + chr->source->data.file.path[0] != '\0') { if (vah_add_file_chardev(&buf, - ctl->def->parallels[i]->source->data.file.path, + chr->source->data.file.path, "rw", - ctl->def->parallels[i]->source->type) != 0) + chr->source->type) != 0) { goto cleanup; + } + } + } + + for (i = 0; i < ctl->def->nchannels; i++) { + virDomainChrDef *chr = ctl->def->channels[i]; - for (i = 0; i < ctl->def->nchannels; i++) - if (ctl->def->channels[i] && - (ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || - ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || - ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || - ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || - ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PIPE) && - ctl->def->channels[i]->source->data.file.path && - ctl->def->channels[i]->source->data.file.path[0] != '\0') + if ((chr->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + chr->source->type == VIR_DOMAIN_CHR_TYPE_DEV || + chr->source->type == VIR_DOMAIN_CHR_TYPE_FILE || + chr->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || + chr->source->type == VIR_DOMAIN_CHR_TYPE_PIPE) && + chr->source->data.file.path && + chr->source->data.file.path[0] != '\0') { if (vah_add_file_chardev(&buf, - ctl->def->channels[i]->source->data.file.path, + chr->source->data.file.path, "rw", - ctl->def->channels[i]->source->type) != 0) + chr->source->type) != 0) { goto cleanup; + } + } + } if (ctl->def->os.kernel) if (vah_add_file(&buf, ctl->def->os.kernel, "r") != 0) @@ -1037,81 +1053,80 @@ get_files(vahControl * ctl) "r") != 0) goto cleanup; - for (i = 0; i < ctl->def->nhostdevs; i++) - if (ctl->def->hostdevs[i]) { - virDomainHostdevDef *dev = ctl->def->hostdevs[i]; + for (i = 0; i < ctl->def->nhostdevs; i++) { + virDomainHostdevDef *dev = ctl->def->hostdevs[i]; - if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - continue; - - switch (dev->source.subsys.type) { - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { - g_autoptr(virUSBDevice) usb = NULL; - - if (virHostdevFindUSBDevice(dev, true, &usb) < 0) - continue; - - if (dev->missing) - continue; + if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; - rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); - if (rc != 0) - goto cleanup; - break; - } - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { - virDomainHostdevSubsysMediatedDev *mdevsrc = &dev->source.subsys.u.mdev; - switch (mdevsrc->model) { - case VIR_MDEV_MODEL_TYPE_VFIO_PCI: - case VIR_MDEV_MODEL_TYPE_VFIO_AP: - case VIR_MDEV_MODEL_TYPE_VFIO_CCW: - needsVfio = true; - break; - case VIR_MDEV_MODEL_TYPE_LAST: - default: - virReportEnumRangeError(virMediatedDeviceModelType, - mdevsrc->model); - break; - } - break; - } + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + g_autoptr(virUSBDevice) usb = NULL; - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { - virPCIDevice *pci = virPCIDeviceNew(&dev->source.subsys.u.pci.addr); - - virDeviceHostdevPCIDriverName driverName = dev->source.subsys.u.pci.driver.name; - - if (driverName == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO || - driverName == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT) { - needsVfio = true; - } - - if (pci == NULL) - continue; + if (virHostdevFindUSBDevice(dev, true, &usb) < 0) + continue; - rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf); - virPCIDeviceFree(pci); + if (dev->missing) + continue; + rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); + if (rc != 0) + goto cleanup; + break; + } + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { + virDomainHostdevSubsysMediatedDev *mdevsrc = &dev->source.subsys.u.mdev; + switch (mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: + needsVfio = true; break; - } - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + case VIR_MDEV_MODEL_TYPE_LAST: default: - rc = 0; + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); break; - } /* switch */ + } + break; } + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + virPCIDevice *pci = virPCIDeviceNew(&dev->source.subsys.u.pci.addr); + + virDeviceHostdevPCIDriverName driverName = dev->source.subsys.u.pci.driver.name; + + if (driverName == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO || + driverName == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT) { + needsVfio = true; + } + + if (pci == NULL) + continue; + + rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf); + virPCIDeviceFree(pci); + + break; + } + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + default: + rc = 0; + break; + } /* switch */ + } + for (i = 0; i < ctl->def->nfss; i++) { - if (ctl->def->fss[i] && - ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT && - (ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH || - ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) && - ctl->def->fss[i]->src) { - virDomainFSDef *fs = ctl->def->fss[i]; + virDomainFSDef *fs = ctl->def->fss[i]; + + if (fs->type == VIR_DOMAIN_FS_TYPE_MOUNT && + (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH || + fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) && + fs->src) { /* We don't need to add deny rw rules for readonly mounts, * this can only lead to troubles when mounting / readonly. @@ -1122,22 +1137,24 @@ get_files(vahControl * ctl) } for (i = 0; i < ctl->def->ninputs; i++) { - if (ctl->def->inputs[i] && - (ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH || - ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_EVDEV)) { + virDomainInputDef *input = ctl->def->inputs[i]; + + if (input->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH || + input->type == VIR_DOMAIN_INPUT_TYPE_EVDEV) { if (vah_add_file(&buf, ctl->def->inputs[i]->source.evdev, "rw") != 0) goto cleanup; } } for (i = 0; i < ctl->def->nnets; i++) { - if (ctl->def->nets[i] && - ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && - ctl->def->nets[i]->data.vhostuser) { + virDomainNetDef *net = ctl->def->nets[i]; + + if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && + net->data.vhostuser) { virDomainChrSourceDef *vhu = ctl->def->nets[i]->data.vhostuser; if (vah_add_file_chardev(&buf, vhu->data.nix.path, "rw", - vhu->type) != 0) + vhu->type) != 0) goto cleanup; } } @@ -1170,10 +1187,11 @@ get_files(vahControl * ctl) } for (i = 0; i < ctl->def->nsysinfo; i++) { + virSysinfoDef *sysinfo = ctl->def->sysinfo[i]; size_t j; - for (j = 0; j < ctl->def->sysinfo[i]->nfw_cfgs; j++) { - virSysinfoFWCfgDef *f = &ctl->def->sysinfo[i]->fw_cfgs[j]; + for (j = 0; j < sysinfo->nfw_cfgs; j++) { + virSysinfoFWCfgDef *f = &sysinfo->fw_cfgs[j]; if (f->file && vah_add_file(&buf, f->file, "r") != 0) @@ -1216,50 +1234,49 @@ get_files(vahControl * ctl) } - if (ctl->def->ntpms > 0) { + for (i = 0; i < ctl->def->ntpms; i++) { + virDomainTPMDef *tpm = ctl->def->tpms[i]; char *shortName = NULL; const char *tpmpath = NULL; - for (i = 0; i < ctl->def->ntpms; i++) { - if (ctl->def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) - continue; - - shortName = virDomainDefGetShortName(ctl->def); - - switch (ctl->def->tpms[i]->data.emulator.version) { - case VIR_DOMAIN_TPM_VERSION_1_2: - tpmpath = "tpm1.2"; - break; - case VIR_DOMAIN_TPM_VERSION_2_0: - tpmpath = "tpm2"; - break; - case VIR_DOMAIN_TPM_VERSION_DEFAULT: - case VIR_DOMAIN_TPM_VERSION_LAST: - break; - } - - /* Unix socket for QEMU and swtpm to use */ - virBufferAsprintf(&buf, - " \"%s/libvirt/qemu/swtpm/%s-swtpm.sock\" rw,\n", - RUNSTATEDIR, shortName); - /* Paths for swtpm to use: give it access to its state - * directory (state files and fsync on dir), log, and PID files. - */ - virBufferAsprintf(&buf, - " \"%s/lib/libvirt/swtpm/%s/%s/\" r,\n", - LOCALSTATEDIR, uuidstr, tpmpath); - virBufferAsprintf(&buf, - " \"%s/lib/libvirt/swtpm/%s/%s/**\" rwk,\n", - LOCALSTATEDIR, uuidstr, tpmpath); - virBufferAsprintf(&buf, - " \"%s/log/swtpm/libvirt/qemu/%s-swtpm.log\" w,\n", - LOCALSTATEDIR, ctl->def->name); - virBufferAsprintf(&buf, - " \"%s/libvirt/qemu/swtpm/%s-swtpm.pid\" rw,\n", - RUNSTATEDIR, shortName); - - VIR_FREE(shortName); + if (tpm->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) + continue; + + shortName = virDomainDefGetShortName(ctl->def); + + switch (tpm->data.emulator.version) { + case VIR_DOMAIN_TPM_VERSION_1_2: + tpmpath = "tpm1.2"; + break; + case VIR_DOMAIN_TPM_VERSION_2_0: + tpmpath = "tpm2"; + break; + case VIR_DOMAIN_TPM_VERSION_DEFAULT: + case VIR_DOMAIN_TPM_VERSION_LAST: + break; } + + /* Unix socket for QEMU and swtpm to use */ + virBufferAsprintf(&buf, + " \"%s/libvirt/qemu/swtpm/%s-swtpm.sock\" rw,\n", + RUNSTATEDIR, shortName); + /* Paths for swtpm to use: give it access to its state + * directory (state files and fsync on dir), log, and PID files. + */ + virBufferAsprintf(&buf, + " \"%s/lib/libvirt/swtpm/%s/%s/\" r,\n", + LOCALSTATEDIR, uuidstr, tpmpath); + virBufferAsprintf(&buf, + " \"%s/lib/libvirt/swtpm/%s/%s/**\" rwk,\n", + LOCALSTATEDIR, uuidstr, tpmpath); + virBufferAsprintf(&buf, + " \"%s/log/swtpm/libvirt/qemu/%s-swtpm.log\" w,\n", + LOCALSTATEDIR, ctl->def->name); + virBufferAsprintf(&buf, + " \"%s/libvirt/qemu/swtpm/%s-swtpm.pid\" rw,\n", + RUNSTATEDIR, shortName); + + VIR_FREE(shortName); } for (i = 0; i < ctl->def->nsmartcards; i++) { -- 2.49.0

From: Michal Privoznik <mprivozn(a)redhat.com&gt; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/virt-aa-helper.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 64cada3b3b..2ea4b47fa5 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -865,7 +865,7 @@ get_files(vahControl * ctl) g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; int rc = -1; size_t i; - char *uuid; + g_autofree char *uuid = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; bool needsVfio = false, needsvhost = false, needsgl = false; @@ -1026,12 +1026,11 @@ get_files(vahControl * ctl) needsgl = true; } else { if (virDomainGraphicsNeedsAutoRenderNode(graphics)) { - char *defaultRenderNode = virHostGetDRMRenderNode(); + g_autofree char *defaultRenderNode = virHostGetDRMRenderNode(); needsgl = true; if (defaultRenderNode) { vah_add_file(&buf, defaultRenderNode, "rw"); - VIR_FREE(defaultRenderNode); } } } @@ -1237,7 +1236,7 @@ get_files(vahControl * ctl) for (i = 0; i < ctl->def->ntpms; i++) { virDomainTPMDef *tpm = ctl->def->tpms[i]; - char *shortName = NULL; + g_autofree char *shortName = NULL; const char *tpmpath = NULL; if (tpm->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) @@ -1276,8 +1275,6 @@ get_files(vahControl * ctl) virBufferAsprintf(&buf, " \"%s/libvirt/qemu/swtpm/%s-swtpm.pid\" rw,\n", RUNSTATEDIR, shortName); - - VIR_FREE(shortName); } for (i = 0; i < ctl->def->nsmartcards; i++) { @@ -1362,7 +1359,6 @@ get_files(vahControl * ctl) ctl->files = virBufferContentAndReset(&buf); cleanup: - VIR_FREE(uuid); return rc; } @@ -1438,15 +1434,13 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) } if (ctl->cmd == 'c' || ctl->cmd == 'r') { - char *xmlStr = NULL; + g_autofree char *xmlStr = NULL; if (virFileReadLimFD(STDIN_FILENO, MAX_FILE_LEN, &xmlStr) < 0) vah_error(ctl, 1, _("could not read xml file")); if (get_definition(ctl, xmlStr) != 0 || ctl->def == NULL) { - VIR_FREE(xmlStr); vah_error(ctl, 1, _("could not get VM definition")); } - VIR_FREE(xmlStr); if (get_files(ctl) != 0) vah_error(ctl, 1, _("invalid VM definition")); -- 2.49.0

From: Michal Privoznik <mprivozn(a)redhat.com&gt; After previous cleanup the cleanup label is no longer necessary. Drop it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/virt-aa-helper.c | 120 ++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 58 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7748a0d19b..b662d971cb 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -863,7 +863,7 @@ static int get_files(vahControl * ctl) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - int rc = -1; + int rc; size_t i; g_autofree char *uuid = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -875,7 +875,7 @@ get_files(vahControl * ctl) if (STRNEQ(uuid, ctl->uuid)) { vah_error(ctl, 0, _("given uuid does not match XML uuid")); - goto cleanup; + return -1; } /* load the storage driver so that backing store can be accessed */ @@ -900,7 +900,7 @@ get_files(vahControl * ctl) /* XXX should handle open errors more careful than just ignoring them. */ if (storage_source_add_files(disk->src, &buf, 0) < 0) - goto cleanup; + return -1; } for (i = 0; i < ctl->def->nserials; i++) { @@ -917,7 +917,7 @@ get_files(vahControl * ctl) chr->source->data.file.path, "rw", chr->source->type) != 0) { - goto cleanup; + return -1; } } } @@ -934,7 +934,7 @@ get_files(vahControl * ctl) chr->source->data.file.path[0] != '\0') { if (vah_add_file(&buf, chr->source->data.file.path, "rw") != 0) { - goto cleanup; + return -1; } } } @@ -953,7 +953,7 @@ get_files(vahControl * ctl) chr->source->data.file.path, "rw", chr->source->type) != 0) { - goto cleanup; + return -1; } } } @@ -972,48 +972,54 @@ get_files(vahControl * ctl) chr->source->data.file.path, "rw", chr->source->type) != 0) { - goto cleanup; + return -1; } } } - if (ctl->def->os.kernel) - if (vah_add_file(&buf, ctl->def->os.kernel, "r") != 0) - goto cleanup; + if (ctl->def->os.kernel && + vah_add_file(&buf, ctl->def->os.kernel, "r") != 0) { + return -1; + } - if (ctl->def->os.initrd) - if (vah_add_file(&buf, ctl->def->os.initrd, "r") != 0) - goto cleanup; + if (ctl->def->os.initrd && + vah_add_file(&buf, ctl->def->os.initrd, "r") != 0) { + return -1; + } - if (ctl->def->os.shim) - if (vah_add_file(&buf, ctl->def->os.shim, "r") != 0) - goto cleanup; + if (ctl->def->os.shim && + vah_add_file(&buf, ctl->def->os.shim, "r") != 0) { + return -1; + } - if (ctl->def->os.dtb) - if (vah_add_file(&buf, ctl->def->os.dtb, "r") != 0) - goto cleanup; + if (ctl->def->os.dtb && + vah_add_file(&buf, ctl->def->os.dtb, "r") != 0) { + return -1; + } for (i = 0; i < ctl->def->os.nacpiTables; i++) { if (vah_add_file(&buf, ctl->def->os.acpiTables[i]->path, "r") != 0) - goto cleanup; + return -1; } - if (ctl->def->pstore) - if (vah_add_file(&buf, ctl->def->pstore->path, "rw") != 0) - goto cleanup; + if (ctl->def->pstore && + vah_add_file(&buf, ctl->def->pstore->path, "rw") != 0) { + return -1; + } if (ctl->def->os.loader && ctl->def->os.loader->path) { bool readonly = false; virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly); if (vah_add_file(&buf, ctl->def->os.loader->path, - readonly ? "rk" : "rwk") != 0) - goto cleanup; + readonly ? "rk" : "rwk") != 0) { + return -1; + } } - if (ctl->def->os.loader && ctl->def->os.loader->nvram) { - if (storage_source_add_files(ctl->def->os.loader->nvram, &buf, 0) < 0) - goto cleanup; + if (ctl->def->os.loader && ctl->def->os.loader->nvram && + storage_source_add_files(ctl->def->os.loader->nvram, &buf, 0) < 0) { + return -1; } for (i = 0; i < ctl->def->ngraphics; i++) { @@ -1023,7 +1029,7 @@ get_files(vahControl * ctl) if (rendernode) { if (vah_add_file(&buf, rendernode, "rw") != 0) - goto cleanup; + return -1; needsgl = true; } else { if (virDomainGraphicsNeedsAutoRenderNode(graphics)) { @@ -1032,7 +1038,7 @@ get_files(vahControl * ctl) if (defaultRenderNode && vah_add_file(&buf, defaultRenderNode, "rw") != 0) { - goto cleanup; + return -1; } } } @@ -1043,15 +1049,15 @@ get_files(vahControl * ctl) if (listenObj.type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET && listenObj.socket && vah_add_file(&buf, listenObj.socket, "rw")) - goto cleanup; + return -1; } } if (ctl->def->ngraphics == 1 && - ctl->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) - if (vah_add_file(&buf, ctl->def->graphics[0]->data.sdl.xauth, - "r") != 0) - goto cleanup; + ctl->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL && + vah_add_file(&buf, ctl->def->graphics[0]->data.sdl.xauth, "r") != 0) { + return -1; + } for (i = 0; i < ctl->def->nhostdevs; i++) { virDomainHostdevDef *dev = ctl->def->hostdevs[i]; @@ -1071,7 +1077,7 @@ get_files(vahControl * ctl) rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); if (rc != 0) - goto cleanup; + return -1; break; } @@ -1132,7 +1138,7 @@ get_files(vahControl * ctl) * this can only lead to troubles when mounting / readonly. */ if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwl", true) != 0) - goto cleanup; + return -1; } } @@ -1142,7 +1148,7 @@ get_files(vahControl * ctl) if (input->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH || input->type == VIR_DOMAIN_INPUT_TYPE_EVDEV) { if (vah_add_file(&buf, ctl->def->inputs[i]->source.evdev, "rw") != 0) - goto cleanup; + return -1; } } @@ -1155,7 +1161,7 @@ get_files(vahControl * ctl) if (vah_add_file_chardev(&buf, vhu->data.nix.path, "rw", vhu->type) != 0) - goto cleanup; + return -1; } } @@ -1165,16 +1171,16 @@ get_files(vahControl * ctl) switch (mem->model) { case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (vah_add_file(&buf, mem->source.nvdimm.path, "rw") != 0) - goto cleanup; + return -1; break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: if (vah_add_file(&buf, mem->source.virtio_pmem.path, "rw") != 0) - goto cleanup; + return -1; break; case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: if (vah_add_file(&buf, DEV_SGX_VEPC, "rw") != 0 || vah_add_file(&buf, DEV_SGX_PROVISION, "r") != 0) { - goto cleanup; + return -1; } break; @@ -1195,7 +1201,7 @@ get_files(vahControl * ctl) if (f->file && vah_add_file(&buf, f->file, "r") != 0) - goto cleanup; + return -1; } } @@ -1206,9 +1212,9 @@ get_files(vahControl * ctl) * model dependent defaults. */ if (shmem->server.enabled && shmem->server.chr->data.nix.path) { - if (vah_add_file(&buf, shmem->server.chr->data.nix.path, - "rw") != 0) - goto cleanup; + if (vah_add_file(&buf, shmem->server.chr->data.nix.path, + "rw") != 0) + return -1; } else { g_autofree char *mem_path = NULL; @@ -1219,18 +1225,18 @@ get_files(vahControl * ctl) break; case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: - /* until exposed, recreate qemuDomainPrepareShmemChardev */ + /* until exposed, recreate qemuDomainPrepareShmemChardev */ mem_path = g_strdup_printf("/var/lib/libvirt/shmem-%s-sock", - shmem->name); + shmem->name); break; case VIR_DOMAIN_SHMEM_MODEL_LAST: virReportEnumRangeError(virDomainShmemModel, shmem->model); break; } - if (mem_path != NULL) { - if (vah_add_file(&buf, mem_path, "rw") != 0) - goto cleanup; + if (mem_path != NULL && + vah_add_file(&buf, mem_path, "rw") != 0) { + return -1; } } } @@ -1353,15 +1359,13 @@ get_files(vahControl * ctl) virBufferAddLit(&buf, " deny \"/var/lib/libvirt/.cache/\" w,\n"); } - if (ctl->newfile) - if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) - goto cleanup; + if (ctl->newfile && + vah_add_file(&buf, ctl->newfile, "rwk") != 0) { + return -1; + } - rc = 0; ctl->files = virBufferContentAndReset(&buf); - - cleanup: - return rc; + return 0; } static int -- 2.49.0