[apparmor] [patch] [parser]: create missing cache directory

[John Johansen]

On 09/27/2013 06:12 PM, Tyler Hicks wrote:
> On 2013-09-23 16:13:49, John Johansen wrote:
>> This patch applies on top of the previous 2 cache patches. It does two
>> things, create the cache dir if it is missing, and moves the cache clearing
>> logic into the create cache routine, because if we are writing a new
>> cache .features file the cache dir should be cleared out.
>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
> 
> Acked-by: Tyler Hicks <tyhicks at canonical.com>
> 
> ... with the disclaimer that I'm not real clear on how the parser does
> its caching. It would be nice if you got an ack from someone else that
> understands this better than I do. However, if you feel confident in

It creates a cache file for each profile file (not includes) it processes.
The compiled binary has the same name as the file it is shadowing and
may contain multiple profiles just like the text file.

In addition there is a .features file that contains the set of features the
profiles were compiled with. Until recently the creation and clearing of
the cache files and features file was all handled by init scripts and what
was stored in the cache and the .features file could get out of sync.

The parser has picked up more and more cache management code to improve
caching, time stamp based dependency checks, clearing of the cache directory,
etc.

The clear cache fn is supposed to remove all files from the cache directory
to help avoid the cache out of sync problem, where the cache files are
compiled for a different kernel than what the .features file is for. Think
of it a new profile gets compiled, and a new .features file gets created
but all the old cache files remain. The idea is if we are going to write a
new .features file then all the contents of the cache directory need to go.

However the clearing is done only for a single directory level, to leave the
option of having subdirectories for different kernel versions in the future.
Long term we should probably be storing a features hash inside the binary
profile cache file to help avoid cache consisency problems, that could
happen if initscripts are managing or the parser fns fail for some reason.

> this patch, it looks correct to me and I think it would be fine to
> commit to trunk as-is.
> 

> 
> As a side note, should we be enforcing cache dir permissions? This patch
hrmm, I am unsure, what is the value of enforcing the permissions?

> creates the dir with a mode of 700. Do we typically expect distro
> packaging to create this dir? If so, maybe we should check their work...

In the past we have relied on the disto creating the directory otherwise
caching just fails.

I think it may be worth making the directory creation an option that
if distros want it could be specified in parser.conf

> 
> Tyler
> 
>>
>> ---
>> === modified file 'parser/parser_main.c'
>> --- parser/parser_main.c	2013-09-23 22:26:51 +0000
>> +++ parser/parser_main.c	2013-09-23 23:09:21 +0000
>> @@ -1260,20 +1260,42 @@
>>  	return error;
>>  }
>>  
>> -static int create_cache(const char *path, const char *features)
>> +static int create_cache(const char *cachedir, const char *path,
>> +			const char *features)
>>  {
>> +	struct stat stat_file;
>>  	FILE * f = NULL;
>>  
>> +	if (cond_clear_cache && clear_cache_files(cacheloc) != 0)
>> +		goto error;
>> +
>> +create_file:
>>  	f = fopen(path, "w");
>>  	if (f) {
>>  		if (fwrite(features, strlen(features), 1, f) != 1 )
>> -			goto fail;
>> +			goto error;
>>  
>>  		fclose(f);
>> -	}
>> -
>> -	return 0;
>> -fail:
>> +
>> +
>> +		return 0;
>> +	}
>> +
>> +error:
>> +	/* does the dir exist? */
>> +	if (stat(cachedir, &stat_file) == -1) {
>> +		if (mkdir(cachedir, 0700) == 0)
>> +			goto create_file;
>> +		if (show_cache)
>> +			PERROR(_("Can't create cache directory: %s\n"), cachedir);
>> +	} else if (!S_ISDIR(stat_file.st_mode)) {
>> +		if (show_cache)
>> +			PERROR(_("File in cache directory location: %s\n"), cachedir);
>> +	} else {
>> +		if (show_cache)
>> +			PERROR(_("Can't update cache directory: %s\n"), cachedir);
>> +	}
>> +
>>  	if (show_cache)
>>  		PERROR("Cache write disabled: cannot create %s\n", path);
>>  	write_cache = 0;
>> @@ -1319,12 +1341,10 @@
>>  	get_flags_string(&cache_flags, cache_features_path);
>>  	if (cache_flags) {
>>  		if (strcmp(flags_string, cache_flags) != 0) {
>> -			if (write_cache && cond_clear_cache) {
>> -				if (clear_cache_files(cacheloc) ||
>> -				    create_cache(cache_features_path,
>> -						 flags_string)) {
>> +			if (write_cache) {
>> +				if (create_cache(cacheloc, cache_features_path,
>> +						 flags_string))
>>  					skip_read_cache = 1;
>> -				}
>>  			} else {
>>  				if (show_cache)
>>  					PERROR("Cache read/write disabled: %s does not match %s\n", FLAGS_FILE, cache_features_path);
>> @@ -1335,7 +1355,7 @@
>>  		free(cache_flags);
>>  		cache_flags = NULL;
>>  	} else if (write_cache) {
>> -		create_cache(cache_features_path, flags_string);
>> +		create_cache(cacheloc, cache_features_path, flags_string);
>>  	}
>>  
>>  	free(cache_features_path);
>>
>>
>>
>> -- 
>> AppArmor mailing list
>> AppArmor at lists.ubuntu.com
>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor