[apparmor] [PATCH 10/11] Fix caching when used with a newer kernel with the feature directory

[John Johansen]

On 03/08/2012 01:46 PM, Kees Cook wrote:
> Hi John,
> 
> On Thu, Mar 08, 2012 at 01:40:54PM -0800, John Johansen wrote:
>> On 03/08/2012 11:17 AM, Steve Beattie wrote:
>>> On Thu, Mar 08, 2012 at 11:15:12AM -0800, Steve Beattie wrote:
>>>> On Wed, Mar 07, 2012 at 06:17:29AM -0800, John Johansen wrote:
>>>>> On newer kernels the features directory causes the creation of a
>>>>> cache/.feature file that contains newline characters.  This causes the
>>>>> feature comparison to fail, because get_flags_string() uses fgets
>>>>> which stop reading in the feature file after the first newline.
>>>>>
>>>>> This caches the features comparision to compare a single line of the
>>>>> file against the full kernel feature directory resulting in caching
>>>>> failure.
>>>>>
>>>>> Worse this also means the cache won't get updated as the parser doesn't
>>>>> change what set gets caches after the .feature file gets created.
>>>>>
>>>>> Signed-off-by: John Johansen <john.johansen at canonical.com>
>>>>
>>>> Acked-By: Steve Beattie <sbeattie at ubuntu.com>
>>>
>>> Actually, fread() doesn't null terminate what it reads like fgets()
>>> does. I think you'll need to address that.
>>>
>> How about with this additional patch on top
>>
>> diff --git a/parser/parser_main.c b/parser/parser_main.c
>> index 67ab231..3f4789b 100644
>> --- a/parser/parser_main.c
>> +++ b/parser/parser_main.c
>> @@ -844,6 +844,7 @@ out:
>>  static void get_flags_string(char **flags, char *flags_file) {
>>  	char *pos;
>>  	FILE *f = NULL;
>> +	size_t size;
>>  
>>  	/* abort if missing or already set */
>>  	if (!flags || *flags)
>> @@ -857,8 +858,10 @@ static void get_flags_string(char **flags, char *flags_file) {
>>  	if (!*flags)
>>  		goto fail;
>>  
>> -	if (!fread(*flags, 1, FLAGS_STRING_SIZE, f))
>> +	size = fread(*flags, 1, FLAGS_STRING_SIZE - 1, f);
>> +	if (ferror(f))
> 
> How about:
> 
>   if (size < 1 || ferror(f))
> ...
> 
> 
>>  		goto fail;
>> +	*flags[size] = 0;
> 
> Then this can't freak out.
> 
hrmmm well it shouldn't freak out if size == 0 and that is the only value less than 1 it
could be, but from read fread again, yeah we should be checking for 0


>>  
>>  	fclose(f);
>>  	pos = strstr(*flags, "change_hat=");
> 
> -Kees
> 
>