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

[John Johansen]

On 07/23/2013 10:43 AM, Tyler Hicks wrote:
> 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.
> 
hrmmm, yeah the warn vs. error is something we should really cleanup and
make consistent