[apparmor] [patch 0/5] [RFC] parser: use alternations for variable expansions

[Steve Beattie]

Hey Seth,

Thanks for all the feedback on this patch set. My responses to your
comments follow:

On Tue, Dec 10, 2013 at 07:29:43PM -0800, Seth Arnold reviewed patch 2/5:
> > -#define STORE(_src, _dest, _len) \
> > -	if ((const char*)_dest + _len > (pcre + pcre_size)){ \
> > -		error = e_buffer_overflow; \
> > -	} else { \
> > -		memcpy(_dest, _src, _len); \
> > -		_dest += _len; \
> > -	}
> 
> This is the only place e_buffer_overflow is ever assigned; please feel
> free to remove its enum and the associated check to see if the buffer
> would have overflowed later on in this function.

Okay, patch queued. There's more error checking code in that function
that could be dropped to simplify things, I think.

It does cause one to realize that we don't have anything catching
an exception if a std::string reallocation failure occurs. We should
fix that.

> >  #define update_re_pos(X) if (!(*first_re_pos)) { *first_re_pos = (X); }
> 
> This might be a good candidate for future cleanups; the only times that
> callers seemed to care about the value passed back in first_re_pos the
> callers were forced to check if the returned type was ePatternBasic and
> re-compute this with strlen(). I haven't actually looked into what it does
> but probably it ought to be hauled out of this function or entirely hauled
> into this function. Anyway...

Yeah, I wasn't entirely clear on what its purpose was, so I left it as
is.

> >  	if (dfaflags & DFA_DUMP_RULE_EXPR)
> > -		fprintf(stderr, "%s\n", pcre);
> > +		fprintf(stderr, "%s\n", pcre.c_str());
> 
> Sooner or later we'll get to replace all these with cerr << :)

Well, we should really just have a consistent messaging interface.

> > +static int build_mnt_opts(std::string& buffer, struct value_list *opts)

[SNIP]

> With only one remaining place where this function can fail, consider
> changing 'goto fail' to return FALSE, ditch the label and other return.

Patch with this fix queued.

On Thu, Dec 12, 2013 at 12:19:19AM -0800, Seth Arnold reviewed patch 2/5:
> > -		if (!convert_entry(devbuf, PATH_MAX +3, NULL))
> > +		devbuf.clear();
> > +		if (!convert_entry(devbuf, NULL))
> 
> I found this idiom, foo.clear(); bar(foo, NULL); to be very
> confusing. Faithful to the original, sure, but I wouldn't be surprised
> if we revisit these before the transition is finally finished. I wish
> I had a better suggestion. _Maybe_ function overloading. Maybe.

Yeah, I didn't really like it, either. My first draft of the conversion
std::string conversion patch had convert_aaregex_to_pcre() clear
the passed in buffer, but because of some of the multi-target stuff,
it needs to append (and I added the unit test bits to ensure that I
didn't come back and add a clear() call to it later).

However, in creating a couple of utility functions, I realized that
what was occurring was that a string buffer would get cleared, the
default match anything pattern would get assigned (copied) to it,
and then that pattern would be passed as an argument in the vector
to the dfa conversion routine. A simpler solution is to just pass a
pointer to the default matching pattern in the vector. I've queued
up a patch that does this.

> > @@ -1058,13 +1039,13 @@ fail:
> >  
> >  static int process_dbus_entry(aare_ruleset_t *dfarules, struct dbus_entry *entry)
> >  {
> > +	char buffer[128];
> 
> 128 is a bit much for what ought to be five characters. :)

Heh :) Really, I'd rather just have found a way to have appended
the constant string to busbuf directly. I have a patch now queued
up that does so using std::ostringstream, but it sure doesn't feel
any prettier.

> >  		test_string = strdup((input)); 								\
> 
> Not introduced, but uncovered, with this patch --
> convert_aaregex_to_pcre()'s first argument is const char *; I didn't see
> that function modifying the argument and I hope the compiler would yell
> if it discovered it -- the strdup() and free() could be dropped here.

Good catch. I've queued a patch that drops test_string and the
associated allocations from both unit test functions.

> > -		ptype = convert_aaregex_to_pcre(test_string, 0, tbuf, PATH_MAX + 3, &pos);		\
> > +		ptype = convert_aaregex_to_pcre(test_string, 0, tbuf, &pos);				\
> >  		asprintf(&output_string, "simple regex conversion for '%s'\texpected = '%s'\tresult = '%s'", \
> > -				(input), expected_str, tbuf);						\
> > -		MY_TEST(strcmp(tbuf, (expected_str)) == 0, output_string);				\
> > +				(input), expected_str, tbuf.c_str());					\
> 
> Also just uncovered -- expected_str here ought to be (expected_str).

Yep, also added to the above patch.

On Thu, Dec 12, 2013 at 01:02:00AM -0800, Seth Arnold reviewed patch 4/5:
> > +	replacement.append(p);
> > +	if (filter_trailing_slash)
> > +		trim_trailing_slash(replacement);
> > +}
> 
> Yuck, reverse iterators _are_ ugly. :)

They really are.

> While trying to come up with something better using substr, I stumbled
> over find_last_not_of() and find_first_not_of():
> http://www.cplusplus.com/reference/string/string/find_first_not_of/
> http://www.cplusplus.com/reference/string/string/find_last_not_of/
> 
> I think these two functions would make it easier to snip the results
> (substr() maybe?) down to something less ugly than modifying a string in
> the middle of a reverse iterator.

Yeah, actually the find_first_not_of() method has a good example of
doing what we want to do. Patch queued.

On Thu, Dec 12, 2013 at 05:24:10PM -0800, Seth Arnold reviewed patch 4/5:
> On Mon, Dec 09, 2013 at 12:37:13PM -0800, Steve Beattie wrote:
> > This patch converts the parser's variable expansion from adding new
> > entries for each additional variable value to incorporating an
> > alternation that includes all the values for the variable
> 
> I think the leading and trailing slashes can probably be handled more
> cleanly, and I think I'd like to see the free(*name) calls pulled up
> towards the start of expand_by_alternations(), but there's nothing worth
> holding up this patch.

Good catch on the free() calls; patch queued.

> Thanks

Thanks for all the reviews!

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20131217/4d17c9ee/attachment.pgp>