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

[Christian Boltz]

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... ;-)

Note to myself: The python code doesn't have support for day(s) and 
week(s), but I'll wait with adding it until the syntax questions in this 
mail are decided because going away from startswith() needs changes 
anyway.

> > +static long long convert_time_units(long long value, long long
> > base, const char *units) +{
> > +	struct time_units *ent;
> > +	for (ent = time_units; ent->str; ent++) {
> > +		if (strcmp(ent->str, units) == 0) {
> > +			if (value * ent->value < base)
> > +				return -1LL;
> > +			return value * ent->value / base;
> > +		}
> > +	}
> > +	return -2LL;
> > +}
> 
> I can haz unit tests?

Feel free to steal some from the python code and testcases ;-)

> > diff --git a/parser/tst/simple_tests/rlimits/ok_rlimit_01.sd
> > b/parser/tst/simple_tests/rlimits/ok_rlimit_01.sd index
> > b13bdd2..8f13257 100644
> > --- a/parser/tst/simple_tests/rlimits/ok_rlimit_01.sd
> > +++ b/parser/tst/simple_tests/rlimits/ok_rlimit_01.sd
> > @@ -1,6 +1,6 @@
> > 
> >  #
> > 
> > -#=DESCRIPTION simple cpu rlimit test
> > -#=EXRESULT PASS
> > +#=DESCRIPTION simple cpu rlimit test, cpu must have units
> > +#=EXRESULT FAIL
> > 
> >  profile rlimit {
> >  
> >    set rlimit cpu <= 1024,

I'm not sure if I like disallowing unit-less rules. This change means we 
possibly break existing profiles - and having an unit-less rlimit rule 
is more likely than having someone with a "set rlimit cpu <= 1024seco" 
rule ;-)

I know it's confusing because cpu and rttime have different default 
units, but that's still better than erroring out. (A warning might be a 
good idea, but the profile should still load.)

Oh, and if you really want to change the behaviour, a follow-up patch to 
rename the affected tests to bad_* would be nice.


Regards,

Christian Boltz
-- 
Wer News über ein Webinterface liest, filmt auch die Tageszeitung,
um sie auf dem Fernseher anzuschauen.        [Henning Schlottmann]