[apparmor] [patch] Let the parser reject ambiguous unit 'm' for rlimit rttime

[John Johansen]

On 06/30/2015 03:34 PM, Christian Boltz wrote:
> Hello,
> 
> Am Montag, 29. Juni 2015 schrieb Steve Beattie:
>> On Thu, Jun 18, 2015 at 02:35:53AM -0700, John Johansen wrote:
>>> Here is a different attempt to fix this
>>>
>>> ---
>>>
>>> commit 0ca80c5efb5ec4b6ba84bcb7e7a8c11afbd1fdf6
>>> Author: John Johansen <john.johansen at canonical.com>
>>> Date:   Thu Jun 18 02:22:42 2015 -0700
>>>
>>>     fix: rlimit unit parsing for time
>>>     
>>>     currently the parser supports ambiguous units like m for time,
>>>     which could mean minutes or milliseconds. Fix this and refactor
>>>     the
>>>     time parsing into a single routine.
>>>     
>>>     Signed-off-by: John Johansen <john.johansen at canonical.com>
>>>
>>> diff --git a/parser/apparmor.d.pod b/parser/apparmor.d.pod
>>> index 9ed5961..1ab8ab0 100644
>>> --- a/parser/apparmor.d.pod
>>> +++ b/parser/apparmor.d.pod
> ...
>>> +B<RLIMIT TIME> = I<NUMBER> ( 'us' | 'microsecond' | 'microseconds'
>>> | 'ms' | 'millisecond' | 'milliseconds' | 's' | 'second' |
>>> 'seconds' | 'min' | 'minute' | 'minutes' | 'h' | 'hour' | 'hours' |
>>> 'day' | 'days' | 'week' | 'weeks' ) Only applies to RLIMIT of
>>> 'cpu', 'rttime',
>> Do we need to note somehow that there can't be any spaces between the
>> number and the time unit:
> 
> Good idea. Please also add a note that cpu only allows >= seconds.
> 
> That said - do we really want it that way, or should we allow spaces for 
> better readability? (Note: for backward compability, the spaces should 
> be optional.)
> 
> While on it - what about the spaces in size rules?
> 
>>> diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
>>> index d529e97..7d238d8 100644
>>> --- a/parser/parser_yacc.y
>>> +++ b/parser/parser_yacc.y
> ...
>>> @@ -881,34 +882,21 @@ rules: rules TOK_SET TOK_RLIMIT TOK_ID TOK_LE
> ...
>>> +				tmp = convert_time_units(tmp, SECONDS_P_MS, 
> end);
>>> +				if (tmp == -1LL)
>>> +					yyerror("RLIMIT '%s %s' < minimum value of 
> 1s\n", $4, $6);
>>> +				else
>>
>> ITYM:                           else if (tmp = -2LL)
>>
>>>  					yyerror("RLIMIT '%s' invalid value %s\n", $4, 
> $6);
>>
>> .. because otherwise there's no way for this to not invoke yyerror.
> 
> Keeping a plain "else" has the advantage that it even catches unexpected 
> errors ;-)  I know the return value of convert_time_units() only has -1 
> and -2 as error values now, but this might change one day, and I prefer 
> to catch that future new error value -3 as well.
> 
>>> @@ -1695,3 +1683,40 @@ mnt_rule *do_pivot_rule(struct cond_entry
>>> *old, char *root, char *transition)> 
>>>  	return ent;
>>>  
>>>  }
>>>
>>> +
>>> +static struct time_units time_units[] = {
>>> +	{ "us", 1LL },
>>> +	{ "microsecond", 1LL },
>>> +	{ "microseconds", 1LL },
>>> +	{ "ms", 1000LL },
>>> +	{ "millisecond", 1000LL },
>>> +	{ "milliseconds", 1000LL },
>>> +	{ "s", 1000LL * 1000LL },
>>
>> "sec" might also be a useful shortcut for second.
>>
>>> +	{ "second", SECONDS_P_MS },
>>> +	{ "seconds", SECONDS_P_MS },
>>> +	{ "min" , 60LL * SECONDS_P_MS },
>>> +	{ "minute", 60LL * SECONDS_P_MS },
>>> +	{ "minutes", 60LL * SECONDS_P_MS },
>>> +	{ "h", 60LL * 60LL * SECONDS_P_MS },
>>> +	{ "hour", 60LL * 60LL * SECONDS_P_MS },
>>> +	{ "hours", 60LL * 60LL * SECONDS_P_MS },
>>> +	{ "day", 24LL * 60LL * 60LL * SECONDS_P_MS },
>>> +	{ "days", 24LL * 60LL * 60LL * SECONDS_P_MS },
>>> +	{ "week", 7LL * 24LL * 60LL * 60LL * SECONDS_P_MS },
>>> +	{ "weekss", 7LL * 24LL * 60LL * 60LL * SECONDS_P_MS },
>>
>> Typo: "weekss" versus "weeks"
> 
> Does this mean the unit has to be one if the units explicitely listed 
> here? 
> 
> The current code allows "s", "se", "sec", "seco", "secon", "second" and 
> "seconds", so this patch means we could break backward compability if 
> someone uses an unexpected short version.
> (We can still argue that this was undocumented behaviour, but... ;-)
> 
yes, it really shouldn't have done that. It will now only support the
options explicitly listed.