On Tue, Oct 22, 2013 at 12:26:36PM -0700, John Johansen wrote: > On 10/22/2013 11:04 AM, Steve Beattie wrote: > > On Sun, Sep 29, 2013 at 02:50:00AM -0700, John Johansen wrote: > >> allow directories to be passed to the parser > >> > >> Allow directories to be passed directly to the parser and handled instead > >> of needing an initscript to find the files in the directory. > >> > >> Signed-off-by: John Johansen <[email protected]> > > > > This patch ends up breaking the parser's ability to take a profile > > on stdin and process it (the simple.pl sanity tests break because of > > it). Is this what we want? > > > fixed version below, it was missing the profilename && condition in main() > > > > - if (stat(profilename, &stat_file) == -1) { > + if (profilename && stat(profilename, &stat_file) == -1) { > PERROR("File %s not found, skipping...\n", profilename); > continue; > } > > - if (stat(profilename, &stat_file) == -1) { > + if (profilename && S_ISDIR(stat_file.st_mode)) {
Ah yes, that seems to have made a difference with the tests.
Some comments inline.
> === modified file 'documentation/AppArmor Develper 1 - Kernel Notes.odt'
> Binary files documentation/AppArmor Develper 1 - Kernel Notes.odt
> 2013-05-02 17:57:23 +0000 and documentation/AppArmor Develper 1 - Kernel
> Notes.odt 2013-08-20 22:30:41 +0000 differ
> === modified file 'documentation/AppArmor Policy.odt'
> Binary files documentation/AppArmor Policy.odt 2013-06-14 19:35:51
> +0000 and documentation/AppArmor Policy.odt 2013-07-26 13:10:32 +0000 differ
The above leaked in to your diff.
> === modified file 'parser/parser.h'
> --- parser/parser.h 2013-09-27 23:16:37 +0000
> +++ parser/parser.h 2013-09-29 09:45:45 +0000
> @@ -271,6 +271,7 @@
> extern void free_var_string(struct var_string *var);
>
> /* parser_misc.c */
> +extern int is_blacklisted(const char *name, const char *path);
> extern struct value_list *new_value_list(char *value);
> extern struct value_list *dup_value_list(struct value_list *list);
> extern void free_value_list(struct value_list *list);
>
> === modified file 'parser/parser_include.c'
> --- parser/parser_include.c 2013-09-27 23:13:22 +0000
> +++ parser/parser_include.c 2013-09-29 09:45:45 +0000
> @@ -279,7 +279,7 @@
>
> struct include_stack_t *include_stack_head = NULL;
>
> -static void start_include_position(char *filename)
> +static void start_include_position(const char *filename)
> {
> if (current_filename)
> free(current_filename);
> @@ -323,7 +323,7 @@
> free(include);
> }
>
> -void reset_include_stack(char *filename)
> +void reset_include_stack(const char *filename)
> {
> while (include_stack_head)
> pop_include_stack();
>
> === modified file 'parser/parser_include.h'
> --- parser/parser_include.h 2012-02-24 12:21:59 +0000
> +++ parser/parser_include.h 2013-09-29 09:45:45 +0000
> @@ -31,6 +31,6 @@
>
> extern void push_include_stack(char *filename);
> extern void pop_include_stack(void);
> -extern void reset_include_stack(char *filename);
> +extern void reset_include_stack(const char *filename);
>
> #endif
>
> === modified file 'parser/parser_lex.l'
> --- parser/parser_lex.l 2013-09-27 23:16:37 +0000
> +++ parser/parser_lex.l 2013-09-29 09:45:45 +0000
> @@ -107,46 +107,6 @@
> #define STATE_TABLE_ENT(X) [(X)] = #X
> /* static char *const state_names[]; */
>
> -struct ignored_suffix_t {
> - const char * text;
> - int len;
> - int silent;
> -};
> -
> -struct ignored_suffix_t ignored_suffixes[] = {
> - /* Debian packging files, which are in flux during install
> - should be silently ignored. */
> - { ".dpkg-new", 9, 1 },
> - { ".dpkg-old", 9, 1 },
> - { ".dpkg-dist", 10, 1 },
> - { ".dpkg-bak", 9, 1 },
> - /* RPM packaging files have traditionally not been silently
> - ignored */
> - { ".rpmnew", 7, 0 },
> - { ".rpmsave", 8, 0 },
> - /* Backup files should be mentioned */
> - { "~", 1, 0 },
> - { NULL, 0, 0 }
> -};
> -
> -static int is_blacklisted(const char *name, const char *path)
> -{
> - int name_len;
> - struct ignored_suffix_t *suffix;
> - name_len = strlen(name);
> - /* skip blacklisted suffixes */
> - for (suffix = ignored_suffixes; suffix->text; suffix++) {
> - char *found;
> - if ( (found = strstr((char *) name, suffix->text)) &&
> - found - name + suffix->len == name_len ) {
> - if (!suffix->silent)
> - PERROR("Ignoring: '%s'\n", path);
> - return 1;
> - }
> - }
> -
> - return 0;
> -}
>
> struct cb_struct {
> const char *fullpath;
>
> === modified file 'parser/parser_main.c'
> --- parser/parser_main.c 2013-09-29 09:44:19 +0000
> +++ parser/parser_main.c 2013-10-22 19:10:32 +0000
> @@ -160,7 +160,7 @@
> "-W, --write-cache Save cached profile (force with -T)\n"
> " --skip-bad-cache Don't clear cache if out of sync\n"
> " --purge-cache Clear cache regardless of its state\n"
> - " --create-cache-dire Create the cache dir if missing\n"
> + " --create-cache-dir Create the cache dir if missing\n"
> "-L, --cache-loc n Set the location of the profile cache\n"
> "-q, --quiet Don't emit warnings\n"
> "-v, --verbose Show profile names as they load\n"
> @@ -821,7 +821,7 @@
> return;
> }
>
> -int process_binary(int option, char *profilename)
> +int process_binary(int option, const char *profilename)
> {
> char *buffer = NULL;
> int retval = 0, size = 0, asize = 0, rsize;
> @@ -882,7 +882,7 @@
> return retval;
> }
>
> -void reset_parser(char *filename)
> +void reset_parser(const char *filename)
> {
> memset(&mru_tstamp, 0, sizeof(mru_tstamp));
> free_aliases();
> @@ -927,7 +927,7 @@
> mru_tstamp = stat_file.st_ctim;
> }
>
> -int process_profile(int option, char *profilename)
> +int process_profile(int option, const char *profilename)
> {
> struct stat stat_bin;
> int retval = 0;
> @@ -952,11 +952,11 @@
>
> if (profilename && option != OPTION_REMOVE) {
> /* make decisions about disabled or complain-mode profiles */
> - basename = strrchr(profilename, '/');
> + basename = (char *) strrchr(profilename, '/');
> if (basename)
> basename++;
> else
> - basename = profilename;
> + basename = (char *) profilename;
>
> if (test_for_dir_mode(basename, "disable")) {
> if (!conf_quiet)
It'd be better to make basename a 'const char *' here, rather than
casting to 'char *', no? We don't modify it the contents pointed
to by basename (even if we change where basename points to), so it
should be safe.
> @@ -1100,6 +1100,48 @@
> return retval;
> }
>
> +/* data - name of parent dir */
> +static int profile_dir_cb(__unused DIR *dir, const char *name, struct stat
> *st,
> + void *data)
> +{
> + int rc = 0;
> +
> + /* skip dot files and files with no name */
> + if (*name == '.' || !strlen(name))
> + return 0;
I kind of want the dot file exclusion to occur in is_blacklisted(). All
current users of is_blacklisted() have a special check for dotfiles
before the call to it already.
> +
> + if (!S_ISDIR(st->st_mode) && !is_blacklisted(name, NULL)) {
> + const char *dirname = (const char *)data;
> + char *path;
> + if (asprintf(&path, "%s/%s", dirname, name) < 0)
> + PERROR(_("Out of memory"));
> + rc = process_profile(option, path);
> + free(path);
> + }
> + return rc;
> +}
> +
> +/* data - name of parent dir */
> +static int binary_dir_cb(__unused DIR *dir, const char *name, struct stat
> *st,
> + void *data)
> +{
> + int rc = 0;
> +
> + /* skip dot files and files with no name */
> + if (*name == '.' || !strlen(name))
> + return 0;
> +
> + if (!S_ISDIR(st->st_mode) && !is_blacklisted(name, NULL)) {
> + const char *dirname = (const char *)data;
> + char *path;
> + if (asprintf(&path, "%s/%s", dirname, name) < 0)
> + PERROR(_("Out of memory"));
> + rc = process_binary(option, name);
Shouldn't this be 'process_binary(option, path)' instead?
> + free(path);
> + }
> + return rc;
> +}
> +
> static int clear_cache_cb(DIR *dir, const char *path, struct stat *st,
> __unused void *data)
> {
The following two chunks leaked into your diff, you already committed
them in trunk rev 2217:
> @@ -1122,7 +1164,7 @@
> struct stat stat_file;
> FILE * f = NULL;
>
> - if (cond_clear_cache && clear_cache_files(cacheloc) != 0)
> + if (clear_cache_files(cacheloc) != 0)
> goto error;
>
> create_file:
> @@ -1197,7 +1239,7 @@
> get_flags_string(&cache_flags, cache_features_path);
> if (cache_flags) {
> if (strcmp(flags_string, cache_flags) != 0) {
> - if (write_cache) {
> + if (write_cache && cond_clear_cache) {
> if (create_cache(cacheloc, cache_features_path,
> flags_string))
> skip_read_cache = 1;
Otherwise, patch looks good.
--
Steve Beattie
<[email protected]>
http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
