On Thu, Oct 7, 2021 at 10:41 PM Ján Tomko <jtomko@xxxxxxxxxx> wrote: > > On a Thursday in 2021, Ioanna Alifieraki wrote: > >This commit aims to address the bug reported in [1] and [2]. > >If the profile is corrupted (0-size) the VM cannot be launched. > >To overcome this check if the profile exists and if it has 0 size > >remove it and create it again. > > > >[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084 > >[2] https://bugs.launchpad.net/bugs/1927519 > > > >Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@xxxxxxxxxxxxx> > >--- > > src/security/virt-aa-helper.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > >diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > >index 5ec0fb8807..5e13b29053 100644 > >--- a/src/security/virt-aa-helper.c > >+++ b/src/security/virt-aa-helper.c > >@@ -1489,6 +1489,7 @@ main(int argc, char **argv) > > int rc = -1; > > char *profile = NULL; > > char *include_file = NULL; > >+ off_t size; > > > > if (virGettextInitialize() < 0 || > > virErrorInitialize() < 0) { > >@@ -1534,6 +1535,28 @@ main(int argc, char **argv) > > if (ctl->cmd == 'c' && virFileExists(profile)) > > vah_error(ctl, 1, _("profile exists")); > > > >+ /* > >+ * Rare cases can leave corrupted empty files behind breaking > >+ * the guest. An empty file is never correct as virt-aa-helper > >+ * would at least add the basic rules, therefore clean this up > >+ * for a proper refresh. > >+ */ > >+ > >+ if (virFileExists(profile)) { > >+ size = virFileLength(profile, -1); > >+ if (size == 0) { > >+ char temp; > >+ vah_warning(_("Profile of 0 size detected, will attempt to remove and re-create it")); > >+ temp = ctl->cmd; > Thank you very much for the feedback provided. I'd like some clarifications. > Please do not pass 'ctl' to the helper functions. It makes it more > diffuclt to see what's going on. Do you suggest this for both remove_profile and create_profile helper functions ? Not passing 'ctl' to the helper functions would make it difficult to tell between the different options. For example, not passing ctl to remove_profile we cannot tell if it's called for D,R or P option. I guess we could overcome this by passing an extra 'cmd' argument and not the 'ctl'. Regarding create_profile 'ctl' is used to check the 'ctl->dryrun' case. Again, an alternative could be an extra argument for the dryrun. What do you think? > > >+ ctl->cmd = 'P'; > >+ if ((rc = remove_profile(ctl, profile, include_file)) != 0) > >+ vah_error(ctl, 1, _("could not remove corrupted profile")); > > Easier to read as: > if (parserRemove(ctl->uuid) < 0) > goto cleanup; > unlink(profile); In that case, I question whether patch 2/4 is needed after all. Adding the '-P' option targets (at least for the time being) that specific case. > > >+ ctl->cmd = temp; > >+ if ((rc = create_profile(ctl, profile, include_file)) != 0) > >+ vah_error(ctl, 1, _("could not re-create profile")); > >+ } > > Since we did not honour ctl->dryrun in the previous step, > it should be safe to call create_profile directly, without the helper > introduced in 1/4. > I'm a bit concerned here, as all tests in virt-aa-helper-test are run with the dryrun option set. Thanks, Jo > Jano > > >+ } > >+ > > if (ctl->append && ctl->newfile) { > > if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) > > goto cleanup; > >-- > >2.17.1 > >
