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

[Tyler Hicks]

One minor nitpick/question...

On 2013-07-23 08:36:12, Steve Beattie wrote:
> Subject: [parser patch] fix apparmor cache tempfile location to use passed arg
> 
> 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). It
> will also break if the filesystem the basedir is located on is mounted
> read-only, which would be a motivating reason to use an alternate
> cache location.
> 
> 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).
> 
> Signed-off-by: Steve Beattie <sbeattie at ubuntu.com>
> 
> ---
>  parser/parser_main.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 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) {
> +				PERROR("Warning failed to write cache: %s\n", cachename);

I've been curious about the policy surrounding -q. Here's the option's
description:

       -q, --quiet
           Do not report on the profiles as they are
           loaded, and not show warnings.

I see there's a pwarn() that respects -q and that there's one PERROR()
call conditional on -q. This message starts with "Warning", so it seems
like it should also respect -q.

(I'm mainly wondering because there's a dbus parser test that I wrote
which spams the build log with the same warning over and over, so this
PERROR() call caught my eye.)

Tyler

> +				unlink(cachetemp);
> +			}
> +			else if (show_cache)
>  				PERROR("Wrote cache: %s\n", cachename);
>  		}
>  		else {
> -- 
> Steve Beattie
> <sbeattie at ubuntu.com>
> http://NxNW.org/~steve/



> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor

-------------- 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/20130723/0d71fba6/attachment-0001.pgp>