Hello, Am Montag, 11. Januar 2016 schrieb John Johansen: > On 01/10/2016 07:22 AM, Christian Boltz wrote: > > Am Freitag, 8. Januar 2016 schrieb John Johansen: > >> diff --git a/devtools/Makefile b/devtools/Makefile > >> new file mode 100644 > >> index 0000000..b0cd26e > >> --- /dev/null > >> +++ b/devtools/Makefile
> >> +print_hfa: print_hfa.o
> >> + $(CC) ${CFLAGS} -o $@ $^ -L
../libraries/libapparmor/src/.libs/
> >> -static -lapparmor
> >
> > Should this -L depend on USE_SYSTEM?
> > (I'm not sure which files are needed, so maybe I'm wrong ;-)
>
> well, not yet. Eventually we will get there but this being the first
> pass I actually wanted to restrict it to the in tree builds atm
I'd still use a variable and set it to "-L ../libraries/..." in both
branches of the "if USE_SYSTEM" to make it obvious that we build in-tree
only.
> >> diff --git a/devtools/expr.txt b/devtools/expr.txt
> >> new file mode 100644
> >> index 0000000..4b7d12b
> >> --- /dev/null
> >> +++ b/devtools/expr.txt
> >> @@ -0,0 +1,772 @@
> >> +1 /
> >> +1 /([^\0000])*\.[Bb][Mm][Pp]
> >> +1 /([^\0000])*\.[Bb][Zz]2
> >> +1 /([^\0000])*\.[Cc][Bb][7RZrz]
> >> +1 /([^\0000])*\.[Dd][Jj][Vv][Uu]
> >> +1 /([^\0000])*\.[Dd][Vv][Ii]
> >
> > What's the reason for using that syntax (looks like "real" regexes
> > or
> > PCRE) instead of AARE?
>
> because it is a PCRE syntax which is more powerful than what AARE can
> express and what is actually used internally on the backend.
>
> > I'd prefer to feed the devtools with AARE, since this is what we use
> > in the profiles. A requirement "variables must be expanded" would
> > be ok.
> Nope, and nope.
>
> This is targeting testing the backend dev work. It allows more and
> will also start allowing "kernel" vars etc which won't be expanded.
Indeed, kernel vars are a good argument to not expect _all_ variables
expanded ;-)
It might still be ok to expect non-kernel vars expanded. (At least for
now - a separate parameter containing variable values might be even
better, but one step after the other ;-)
> Eventually we may open up some of the PCRE syntax to the front end
> profiles, certainly if not the syntax some of the capabilities.
>
> We do have some unit tests for converting between the two, but more
> tests are always good
There are some tests (including expected matches) in the python tests
(test-aare.py TestConvert_regexpAndAAREMatch) that test the python AARE
implementation.
If you provide a way to convert them (or even to run everything from
python), we can run the same set of tests against the libapparmor/future
kernel AARE checking.
Yes, I know that won't happen in the next days. Just keep it in mind as
something to do in the future.
> >> diff --git a/devtools/print_hfa.c b/devtools/print_hfa.c
> >> new file mode 100644
> >> index 0000000..c5a5b65
> >> --- /dev/null
> >> +++ b/devtools/print_hfa.c
> >>
> >> +const char *short_options = "";
> >> +struct option long_options[] = {
> >> + {NULL, 0, 0, 0},
> >> +};
> >> +
> >> +static int process_args(int argc, char *argv[])
> > Also, how can the error condition (case 0) happen? From reading
> > getopt_long(3), I'd guess it happens if a long option is matched -
> > but it's very unlikely that NULLL will ever match ;-)
> > (Also, why do we need that NULL in long_options at all?)
>
> the null is a terminator, without it you can get strange option
> processing based on random junk in memory, and yes we had a bug
> related to this once
Sounds interesting[tm] ;-) (It also sounds like a bug in getopt_long,
but if the workaround is that easy, we should of course keep it as
safety net. Nevertheless, I hope this is or gets fixed in getopt_long.)
> > Reading a file into a buffer doesn't look that easy in C ;-)
> >
> > Is there really no function available somewhere that shortens all
> > this to something like
> >
> > buffer = read_file(filename)
> >
> > + checking errno?
>
> meh, there are alternatives, like using fopen and fread, mmap, ...
>
> I'm not sure why I through in a more low level approach here. It can
> be changed
That would be a good idea - I already found two double-close(), and
someone who really understands C might find even more interesting things.
> >> +char *load_file(const char *path)
> >
> > That looks like the same load_file() as in print_hfa.c, therefore
> > - please avoid copy&paste programming and move it to a separate file
> >
> > that can be used by both binaries
>
> no, this really does not warrant sharing, especially not at this time
copy&paste programming is never a good idea IMHO, but well, _I_ will not
suffer the pain from fixing it ;-)
Hint: you already have to copy over the double-close() fix...
> >> diff --git a/libraries/libapparmor/src/hfa.c
> >> b/libraries/libapparmor/src/hfa.c new file mode 100644
> >> index 0000000..ec31a15
> >> --- /dev/null
> >> +++ b/libraries/libapparmor/src/hfa.c
> > I don't say that I completely understand this, but a short and
> > simplified summary seems to be that this code does matching like
> > the kernel does when deciding if an action is allowed/denied.
> > Right?
>
> yes
>
> > Does this also mean this code is "stolen" from the kernel code?
>
> Largely it is a fresh rewrite, and the kernel will be picking up this
> code once its ready. There are a few bits and pieces from the kernel
> code, largely around the defines in the .h files
Ah, good to know.
Will this enter the kernel before or after ptrace, signal etc. handling?
*SCNR*
> >> diff --git a/libraries/libapparmor/swig/SWIG/libapparmor.i
> >> b/libraries/libapparmor/swig/SWIG/libapparmor.i index
> >> 69b4cc2..47213a1 100644
> >> --- a/libraries/libapparmor/swig/SWIG/libapparmor.i
> >> +++ b/libraries/libapparmor/swig/SWIG/libapparmor.i
> > It's nice to see more library functions exported towards the swig
> > bindings.
>
> ah crud I missed ripping those out :)
>
> we aren't ready to be exporting these functions yet.
>
> > Now to the practical question - are some of those functions useful
> > to, let's say, handle the AARE matching in the tools?
>
> Not yet, but that is the goal
:-)
Also, thanks for explaining the less obvious parts of the code ;-)
Regards,
Christian Boltz
--
Was habt Ihr denn? emacs ist doch ein tolles Betriebssystem!
Das einzige was ihm fehlt, ist ein vernünftiger Editor (vim?)
[Jan Trippler in suse-linux]
signature.asc
Description: This is a digitally signed message part.
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
