[apparmor] [parser patch] fix apparmor cache tempfile location to use passed arg v2

[Steve Beattie]

On Thu, Jul 25, 2013 at 07:17:19PM -0700, Seth Arnold wrote:
> Acked-by: Seth Arnold <seth.arnold at canonical.com>

In case people were wondering why I hadn't committed this yet,
I self-NACKed my patch because I had committed the unpardonable sin
of not writing a testcase to demonstrate the issue and to address
Tyler's question about PERROR vs. pwarn() for this specific case.
Below is v2 of the patch with a couple of testcases added and the
conversion to pwarn() so that -q will silence the warning if it failed
to rename() the cache file into place.

Also, submitting this for 2.8 as well. (It should be noted that Ubuntu
has incorporated v1 of this patch in the saucy devel release.)

Subject: [patch] fix apparmor cache tempfile location to use passed arg v2

This patch fixes problems in the handling of both the final cache
name location and the temporary cache file when an alternate location
is specified.

The first issue is that if the alternate cache directory location was
specified, the alternate directory name would be used as the final location for
the cache file, rather than the alternate directory + the basename of
the profile.

The second issue is that it would generate the temporary file that it
stores the cache file in [basedir]/cache even if an alternate cache
location was specified on the command line. This causes a problem
if [basedir]/cache is on a separate device than the alternate cache
location, because the rename() of the tempfile into the final location
would fail (which the parser would not check the return code of).

This patch fixes the above by incorporating the basename into the cache
file name if the alternate cache location has been specified, bases the
temporary cache file name on the destination cache name (such that they
end up in the same directory), and finally detects if the rename fails
and unlinks the temporary file if that happens (rather than leave it
around). It also has been updated to add a couple of testcases to verify
that writing and reading from an alternate cache location work.

Patch history:
  v1: first draft of patch
  v2: add testcases, convert PERROR() to pwarn() if rename() fails for
      placing cachefile into place.

Signed-off-by: Steve Beattie <sbeattie at ubuntu.com>

---
 parser/parser_main.c  |   12 +++++++-----
 parser/tst/caching.sh |   13 ++++++++++++-
 2 files changed, 19 insertions(+), 6 deletions(-)

Index: b/parser/parser_main.c
===================================================================
--- a/parser/parser_main.c
+++ b/parser/parser_main.c
@@ -1069,8 +1069,7 @@ int process_profile(int option, char *pr
 	if ((profilename && option != OPTION_REMOVE) && !force_complain &&
 	    !skip_cache) {
 		if (cacheloc) {
-			cachename = strdup(cacheloc);
-			if (!cachename) {
+			if (asprintf(&cachename, "%s/%s", cacheloc, basename)<0) {
 				PERROR(_("Memory allocation error."));
 				exit(1);
 			}
@@ -1089,7 +1088,7 @@ int process_profile(int option, char *pr
 		}
 		if (write_cache) {
 			/* Otherwise, set up to save a cached copy */
-			if (asprintf(&cachetemp, "%s/%s/%s-XXXXXX", basedir, "cache", basename)<0) {
+			if (asprintf(&cachetemp, "%s-XXXXXX", cachename)<0) {
 				perror("asprintf");
 				exit(1);
 			}
@@ -1147,8 +1146,11 @@ out:
 		}
 
 		if (useable_cache) {
-			rename(cachetemp, cachename);
-			if (show_cache)
+			if (rename(cachetemp, cachename) < 0) {
+				pwarn("Warning failed to write cache: %s\n", cachename);
+				unlink(cachetemp);
+			}
+			else if (show_cache)
 				PERROR("Wrote cache: %s\n", cachename);
 		}
 		else {
Index: b/parser/tst/caching.sh
===================================================================
--- a/parser/tst/caching.sh
+++ b/parser/tst/caching.sh
@@ -14,7 +14,8 @@ APPARMOR_PARSER="${APPARMOR_PARSER:-../a
 
 # fake base directory
 basedir=$(mktemp -d -t aa-cache-XXXXXX)
-trap "rm -rf $basedir" EXIT
+altcachedir=$(mktemp -d -t aa-alt-cache-XXXXXXXX)
+trap "rm -rf $basedir $altcachedir" EXIT
 mkdir -p $basedir/cache
 
 ARGS="--base $basedir --skip-kernel-load"
@@ -160,3 +161,13 @@ echo "ok"
 echo -n "Cache reading is skipped when parser in \$PATH is newer: "
 (PATH=$basedir/parser/ /bin/sh -c "apparmor_parser $ARGS -v -r $basedir/$profile") | grep -q 'Replacement succeeded for' || { echo "FAIL"; exit 1; }
 echo "ok"
+
+echo -n "Profiles are cached in alternate location when requested: "
+${APPARMOR_PARSER} $ARGS -q --write-cache --cache-loc $altcachedir -r $basedir/$profile
+[ ! -f $altcachedir/$profile ] && echo "FAIL ($altcachedir/$profile does not exist)" && exit 1
+echo "ok"
+
+echo -n "Cache is loaded from alt location when it exists and features match: "
+${APPARMOR_PARSER} $ARGS -v -r $basedir/$profile --cache-loc $altcachedir | grep -q 'Cached reload succeeded' || { echo "FAIL"; exit 1; }
+echo "ok"
+

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130725/5337347b/attachment-0001.pgp>