[apparmor] [Patch][parser]

[Seth Arnold]

On Fri, Sep 19, 2014 at 12:27:21PM -0700, John Johansen wrote:
> fix: Make the parser behave the same as when driven with xargs -n1
> 
> Currently the parser is bailing when it fails to load a profile,
> not processing any potential subsequent profiles in the dir or passed
> in list. This results in all policy after the first error failing
> to load, instead of just the profile(s) with the error.
> 
> This is a different behavior than what has been done by initscripts
> that have driven it with xargs -n1, passing it a single profile
> at a time.
> 
> Fix this so that the parser only exits on first error if specifically
> told to do so.
> 
> Note: this does not fix the various failure points in the parser
> that call exit, instead of returning an error.
> 
> Signed-off-by: John Johansen <john.johansen at canonical.com>

I believe this will address the failures that Jamie's seen. I'm .. on the
fence about the idea as a whole but I believe this implementation is good.

Acked-by: Seth Arnold <seth.arnold at canonical.com>

Thanks

> ---
>  apparmor/2.9-new/parser/parser_main.c |   31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> --- jj.orig/apparmor/2.9-new/parser/parser_main.c
> +++ jj/apparmor/2.9-new/parser/parser_main.c
> @@ -74,6 +74,7 @@
>  int create_cache_dir = 0;		/* create the cache dir if missing? */
>  int preprocess_only = 0;
>  int skip_mode_force = 0;
> +int abort_on_error = 0;			/* stop processing profiles if error */
>  struct timespec mru_tstamp;
>  
>  #define FEATURES_STRING_SIZE 8192
> @@ -123,6 +124,7 @@
>  	{"optimize",		1, 0, 'O'},
>  	{"Optimize",		1, 0, 'O'},
>  	{"preprocess",		0, 0, 'p'},
> +	{"abort-on-error",	0, 0, 132},	/* no short option */
>  	{NULL, 0, 0, 0},
>  };
>  
> @@ -172,6 +174,7 @@
>  	       "-D [n], --dump		Dump internal info for debugging\n"
>  	       "-O [n], --Optimize	Control dfa optimizations\n"
>  	       "-h [cmd], --help[=cmd]  Display this text or info about cmd\n"
> +	       "--abort-on-error	abort processing of profiles on first error\n"
>  	       ,command);
>  }
>  
> @@ -410,6 +413,9 @@
>  	case 131:
>  		create_cache_dir = 1;
>  		break;
> +	case 132:
> +		abort_on_error = 1;
> +		break;
>  	case 'L':
>  		cacheloc = strdup(optarg);
>  		break;
> @@ -718,9 +724,10 @@
>  	if (profilename) {
>  		fd = open(profilename, O_RDONLY);
>  		if (fd == -1) {
> +			retval = errno;
>  			PERROR(_("Error: Could not read binary profile or cache file %s: %s.\n"),
>  			       profilename, strerror(errno));
> -			exit(errno);
> +			return retval;
>  		}
>  	} else {
>  		fd = dup(0);
> @@ -733,7 +740,7 @@
>  			chunksize <<= 1;
>  			if (!buffer) {
>  				PERROR(_("Memory allocation error."));
> -				exit(errno);
> +				return ENOMEM;
>  			}
>  		}
>  
> @@ -859,7 +866,7 @@
>  		if ( !(yyin = fopen(profilename, "r")) ) {
>  			PERROR(_("Error: Could not read profile %s: %s.\n"),
>  			       profilename, strerror(errno));
> -			exit(errno);
> +			return errno;
>  		}
>  	}
>  	else {
> @@ -921,7 +928,7 @@
>  	    !skip_cache) {
>  		if (asprintf(&cachename, "%s/%s", cacheloc, basename)<0) {
>  			PERROR(_("Memory allocation error."));
> -			exit(1);
> +			return ENOMEM;
>  		}
>  		/* Load a binary cache if it exists and is newest */
>  		if (!skip_read_cache &&
> @@ -937,11 +944,11 @@
>  			/* Otherwise, set up to save a cached copy */
>  			if (asprintf(&cachetemp, "%s-XXXXXX", cachename)<0) {
>  				perror("asprintf");
> -				exit(1);
> +				return ENOMEM;
>  			}
>  			if ( (cache_fd = mkstemp(cachetemp)) < 0) {
>  				perror("mkstemp");
> -				exit(1);
> +				return ENOMEM;
>  			}
>  		}
>  	}
> @@ -1203,12 +1210,14 @@
>  	setup_flags();
>  
>  	retval = 0;
> -	for (i = optind; retval == 0 && i <= argc; i++) {
> +	for (i = optind; i <= argc; i++) {
>  		struct stat stat_file;
>  
>  		if (i < argc && !(profilename = strdup(argv[i]))) {
>  			perror("strdup");
> -			return -1;
> +			if (abort_on_error)
> +				return -1;
> +			continue;
>  		}
>  		/* skip stdin if we've seen other command line arguments */
>  		if (i == argc && optind != argc)
> @@ -1223,10 +1232,9 @@
>  			int (*cb)(DIR *dir, const char *name, struct stat *st,
>  				  void *data);
>  			cb = binary_input ? binary_dir_cb : profile_dir_cb;
> -			if (dirat_for_each(NULL, profilename, profilename, cb)) {
> +			if ((retval = dirat_for_each(NULL, profilename, profilename, cb))) {
>  				PDEBUG("Failed loading profiles from %s\n",
>  				       profilename);
> -				exit(1);
>  			}
>  		} else if (binary_input) {
>  			retval = process_binary(option, profilename);
> @@ -1236,6 +1244,9 @@
>  
>  		if (profilename) free(profilename);
>  		profilename = NULL;
> +
> +		if (abort_on_error && retval != 0)
> +			break;
>  	}
>  
>  	if (ofile)
> 
> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140919/bd4569ed/attachment.pgp>