[apparmor] [patch 03/26] fix failure paths around policy that can result in a crash

[John Johansen]

On 04/15/2014 04:48 PM, Seth Arnold wrote:
> On Tue, Apr 15, 2014 at 10:22:10AM -0700, john.johansen at canonical.com wrote:
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
>> Acked-by: Steve Beattie <steve at nxnw.org>
>>
> 
> There's a lot of extra code duplication here. I don't particularly like
> the way this thing turned out.. it's more obvious with the full code, I'll
> paste it in below.
> 
>> ---
>>  parser/parser_regex.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> --- 2.9-test.orig/parser/parser_regex.c
>> +++ 2.9-test/parser/parser_regex.c
>> @@ -712,6 +712,9 @@
>>  		prof->policy.rules = NULL;
>>  		if (!prof->policy.dfa)
>>  			goto out;
>> +	} else {
>> +		aare_delete_ruleset(prof->policy.rules);
>> +		prof->policy.rules = NULL;
>>  	}
>>  
>>  	aare_reset_matchflags();
>> @@ -719,6 +722,9 @@
>>  	error = 0;
>>  
>>  out:
>> +	aare_delete_ruleset(prof->policy.rules);
>> +	prof->policy.rules = NULL;
>> +
>>  	return error;
>>  }
> 
> 
> # marks duplicated code
> ! marks a function that is called within the aare_delete_ruleset() function
> 
> I don't have a rewritten version handy to suggest, but this feels like
> prime candidate for being cut in half.
> 
>         if (prof->policy.count > 0) {
>                 prof->policy.dfa = aare_create_dfa(prof->policy.rules,
>                                                   &prof->policy.size,
>                                                   dfaflags);
> #               aare_delete_ruleset(prof->policy.rules);
> #               prof->policy.rules = NULL;
>                 if (!prof->policy.dfa)
>                         goto out;
>         } else {
> #               aare_delete_ruleset(prof->policy.rules);
> #               prof->policy.rules = NULL;
>         }
> 
> !       aare_reset_matchflags();
> 
>         error = 0;
> 
> out:
> #       aare_delete_ruleset(prof->policy.rules);
> #       prof->policy.rules = NULL;
> 
>         return error;
> }
> 

we could do

        if (prof->policy.count > 0) {
                prof->policy.dfa = aare_create_dfa(prof->policy.rules,
                                                  &prof->policy.size,
                                                  dfaflags);
                if (!prof->policy.dfa)
                        goto out;
        }

#       aare_delete_ruleset(prof->policy.rules);
#       prof->policy.rules = NULL;

!       aare_reset_matchflags();

        error = 0;

out:
#       aare_delete_ruleset(prof->policy.rules);
#       prof->policy.rules = NULL;

        return error;

which removes one of the duplicates.