apparmor may fail to load some profiles if one is corrupted

The cause of the corruption is believed to be an interaction between the click-system-hooks and the apparmor upstart jobs. click-apparmor will be adjusted to use a blocking lockfile to avoid the corruption. As such, the apparmor task priority can be reduced.

After discussing with the apparmor team, fixing the parser bug can (and should be done) but it more involved that the cache bug and we can't fix it in time for rtm. If the lockfile doesn't fully address this issue, we can go back to using '-n1' with xargs unconditionally in /lib/apparmor/functions.

Upon further investigation, python3-apparmor-click and python3-apparmor-easyprof both use shutil.move() to put a temp file into place. shutil.move() will use os.rename() if the files reside on the same file, but will use shutil.copy2() followed by an unlink otherwise. Since the tempfile.mkstemp() in both cases does not specify to use a different temp directory (ie, dir=None), these files will be created in /tmp, which is a tmpfs on devices (verified on mako), therefore the shutil.move() is not atomic. This confirms that utilizing a blocking lock file will prevent at least some forms of races and corruption. We could adjust the mkstemp() call to use the same filesystem, however, that would result in unexpected behavior when two aa-clickhooks are run at the same time (ie, both would think they did everything correctly but each could have missed something).

For rtm I can add a workaround to /lib/apparmor/functions to fallback to using -n1 if tha parser fails on the profile set. This is a minimal change and retains the performance improvements of not using -n1 in the normal case of things being ok. However, we want to remove this and rely on the parser handling this correctly going forward.

This bug was fixed in the package click-apparmor - 0.2.11.1

---------------
click-apparmor (0.2.11.1) utopic; urgency=medium

  * aa-clickhook: don't remove the lock file so we can properly handle 3 or
    more processes contending for the lock

click-apparmor (0.2.11) utopic; urgency=medium

  * apparmor/click.py: be more careful with out_fn assignment in
    output_policy()
  * aa-clickhook: implement blocking lockfile to make sure this script does
    not run concurrently with itself (LP: #1377338)
 -- Jamie Strandboge <email address hidden> Tue, 07 Oct 2014 09:32:53 -0500

This bug was fixed in the package apparmor - 2.8.96~2652-0ubuntu5.1

---------------
apparmor (2.8.96~2652-0ubuntu5.1) 14.09; urgency=medium

  * debian/apparmor.{upstart,init}: check if click-apparmor md5sums changed so
    we regenerate the policy if it changes too (LP: #1371574)
  * debian/lib/apparmor/functions: fall back to using -n1 if the parser failed
    to load a profile set. This should be removed when the parser properly
    handles profile sets with corrupted profiles (LP: #1377338).
 -- Jamie Strandboge <email address hidden> Tue, 07 Oct 2014 09:24:45 -0500

14.10 had workaround in place in 2.8.98-0ubuntu2

Along with LP: #1488179, this is one source of ugliness in current Debian/Ubuntu initscript, that makes it harder than needed to port it to systemd.

I'm a bit confused:

* On the one hand, this bug is *not* marked is fixed in AppArmor upstream; the only reason it was marked as "Fix Released" for Ubuntu is the pile of kludges added in /lib/apparmor/functions, that I migrated to rc.apparmor.functions upstream a few years back.

* On the other hand, the aforementioned pile of kludges was removed by https://gitlab.com/apparmor/apparmor/-/commit/0b8ea047e88b250862da73a968b1cd1f8b7f6b91 because "LP:1377338 has been fixed for quite awhile".

So, it seems to me that:

* Either the parser bug was actually fixed upstream, and then the status this bug is incorrect: it should be "Fix Released".
* Or the parser bug is still there, and then 0b8ea047e88b250862da73a968b1cd1f8b7f6b91 was done based on a misunderstanding.

Which is it?

It should be fixed as of the AppArmor 3.0 release. With 3.0 the handling of jobs doesn't stop with an error unless --abort-on-error is specified. Instead the parser will keep track of the last error and return that there was an error, but it will keep processing the rest of the jobs.

We did not close this for 3.0 as we wanted more time, to make sure we have it fixed. But we are considering it fixed on the dev branch. Though christian did turn up another corner case the other day https://gitlab.com/apparmor/apparmor/-/issues/215 that we need to finish fixing.