checkpolicy: Add --werror flag to checkmodule and checkpolicy to treat warnings as errors.
diff mbox series

Message ID 20200305184034.165554-1-dburgener@linux.microsoft.com
State Accepted
Headers show
Series
  • checkpolicy: Add --werror flag to checkmodule and checkpolicy to treat warnings as errors.
Related show

Commit Message

Daniel Burgener March 5, 2020, 6:40 p.m. UTC
When the lexer encounters an unexpected character in a policy source file, it prints a warning, discards the character and moves on.  In some build environments, these characters could be a symptom of an earlier problem, such as unintended results of expansion of preprocessor macros, and the ability to have the compiler halt on such issues would be helpful for diagnosis.

Signed-off-by: Daniel Burgener <Daniel.Burgener@microsoft.com>
---
 checkpolicy/checkmodule.8 |  3 +++
 checkpolicy/checkmodule.c | 10 ++++++++--
 checkpolicy/checkpolicy.8 |  3 +++
 checkpolicy/checkpolicy.c |  9 +++++++--
 checkpolicy/policy_scan.l |  4 ++++
 5 files changed, 25 insertions(+), 4 deletions(-)

Comments

Stephen Smalley March 6, 2020, 1:57 p.m. UTC | #1
On Thu, Mar 5, 2020 at 1:40 PM Daniel Burgener
<dburgener@linux.microsoft.com> wrote:
>
> When the lexer encounters an unexpected character in a policy source file, it prints a warning, discards the character and moves on.  In some build environments, these characters could be a symptom of an earlier problem, such as unintended results of expansion of preprocessor macros, and the ability to have the compiler halt on such issues would be helpful for diagnosis.
>
> Signed-off-by: Daniel Burgener <Daniel.Burgener@microsoft.com>

I'm trying to remember why this particular case (unmatched character
in the lexer) isn't already a fatal error.
If there isn't a real reason for it, we could alternatively just
switch it to use yyerror() in that case.
Otherwise, your description suggests that you only want to make that
particular case a fatal error; are you sure
you want to treat all warnings as fatal errors?
Daniel Burgener March 6, 2020, 2:16 p.m. UTC | #2
On 3/6/20 8:57 AM, Stephen Smalley wrote:
> On Thu, Mar 5, 2020 at 1:40 PM Daniel Burgener 
> <dburgener@linux.microsoft.com> wrote:
>> When the lexer encounters an unexpected character in a policy source 
>> file, it prints a warning, discards the character and moves on. In 
>> some build environments, these characters could be a symptom of an 
>> earlier problem, such as unintended results of expansion of 
>> preprocessor macros, and the ability to have the compiler halt on 
>> such issues would be helpful for diagnosis. Signed-off-by: Daniel 
>> Burgener <Daniel.Burgener@microsoft.com> 
> I'm trying to remember why this particular case (unmatched character 
> in the lexer) isn't already a fatal error. If there isn't a real 
> reason for it, we could alternatively just switch it to use yyerror() 
> in that case. Otherwise, your description suggests that you only want 
> to make that particular case a fatal error; are you sure you want to 
> treat all warnings as fatal errors?

That's a bug in the description rather than the code.  That particular 
case is what caused me to want this option, but my intent was to add an 
ability to treat all warnings as errors.  I can resubmit with a better 
message if you'd like.

I'm not sure why the behavior is that that particular case is a 
warning.  Git blame shows it as going back to the import from svn, so 
it's been that way for a while.  I'd think that --werror would be 
helpful either way.  If no one has a reason for this particular case to 
be a warning rather than an error, I can submit a patch for that change 
as well.

-Daniel
James Carter March 6, 2020, 3:44 p.m. UTC | #3
It looks to me like the only warnings are for:
- Conflicting expandattribute rules (which will default to false)
- A dontaudit rule that uses ~ for its permissions
- Stating that role dominance is deprecated
- Unexpected character: @ $ % & _ + = \ | " ' < > ?

It might make more sense just to make the unexpected character case to
always be an error.

Jim

On Fri, Mar 6, 2020 at 9:16 AM Daniel Burgener
<dburgener@linux.microsoft.com> wrote:
>
> On 3/6/20 8:57 AM, Stephen Smalley wrote:
> > On Thu, Mar 5, 2020 at 1:40 PM Daniel Burgener
> > <dburgener@linux.microsoft.com> wrote:
> >> When the lexer encounters an unexpected character in a policy source
> >> file, it prints a warning, discards the character and moves on. In
> >> some build environments, these characters could be a symptom of an
> >> earlier problem, such as unintended results of expansion of
> >> preprocessor macros, and the ability to have the compiler halt on
> >> such issues would be helpful for diagnosis. Signed-off-by: Daniel
> >> Burgener <Daniel.Burgener@microsoft.com>
> > I'm trying to remember why this particular case (unmatched character
> > in the lexer) isn't already a fatal error. If there isn't a real
> > reason for it, we could alternatively just switch it to use yyerror()
> > in that case. Otherwise, your description suggests that you only want
> > to make that particular case a fatal error; are you sure you want to
> > treat all warnings as fatal errors?
>
> That's a bug in the description rather than the code.  That particular
> case is what caused me to want this option, but my intent was to add an
> ability to treat all warnings as errors.  I can resubmit with a better
> message if you'd like.
>
> I'm not sure why the behavior is that that particular case is a
> warning.  Git blame shows it as going back to the import from svn, so
> it's been that way for a while.  I'd think that --werror would be
> helpful either way.  If no one has a reason for this particular case to
> be a warning rather than an error, I can submit a patch for that change
> as well.
>
> -Daniel
>
Stephen Smalley March 9, 2020, 1:42 p.m. UTC | #4
On Fri, Mar 6, 2020 at 9:16 AM Daniel Burgener
<dburgener@linux.microsoft.com> wrote:
>
> On 3/6/20 8:57 AM, Stephen Smalley wrote:
> > On Thu, Mar 5, 2020 at 1:40 PM Daniel Burgener
> > <dburgener@linux.microsoft.com> wrote:
> >> When the lexer encounters an unexpected character in a policy source
> >> file, it prints a warning, discards the character and moves on. In
> >> some build environments, these characters could be a symptom of an
> >> earlier problem, such as unintended results of expansion of
> >> preprocessor macros, and the ability to have the compiler halt on
> >> such issues would be helpful for diagnosis. Signed-off-by: Daniel
> >> Burgener <Daniel.Burgener@microsoft.com>
> > I'm trying to remember why this particular case (unmatched character
> > in the lexer) isn't already a fatal error. If there isn't a real
> > reason for it, we could alternatively just switch it to use yyerror()
> > in that case. Otherwise, your description suggests that you only want
> > to make that particular case a fatal error; are you sure you want to
> > treat all warnings as fatal errors?
>
> That's a bug in the description rather than the code.  That particular
> case is what caused me to want this option, but my intent was to add an
> ability to treat all warnings as errors.  I can resubmit with a better
> message if you'd like.
>
> I'm not sure why the behavior is that that particular case is a
> warning.  Git blame shows it as going back to the import from svn, so
> it's been that way for a while.  I'd think that --werror would be
> helpful either way.  If no one has a reason for this particular case to
> be a warning rather than an error, I can submit a patch for that change
> as well.

Ok.  I have no objections to your patch, just wanted to make sure it
is doing what you really want it to do.
I'd also be willing to accept a patch to make unexpected characters fatal.
Stephen Smalley March 9, 2020, 1:58 p.m. UTC | #5
On Thu, Mar 5, 2020 at 1:40 PM Daniel Burgener
<dburgener@linux.microsoft.com> wrote:
>
> When the lexer encounters an unexpected character in a policy source file, it prints a warning, discards the character and moves on.  In some build environments, these characters could be a symptom of an earlier problem, such as unintended results of expansion of preprocessor macros, and the ability to have the compiler halt on such issues would be helpful for diagnosis.
>
> Signed-off-by: Daniel Burgener <Daniel.Burgener@microsoft.com>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Stephen Smalley March 11, 2020, 6:40 p.m. UTC | #6
On Mon, Mar 9, 2020 at 9:58 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Mar 5, 2020 at 1:40 PM Daniel Burgener
> <dburgener@linux.microsoft.com> wrote:
> >
> > When the lexer encounters an unexpected character in a policy source file, it prints a warning, discards the character and moves on.  In some build environments, these characters could be a symptom of an earlier problem, such as unintended results of expansion of preprocessor macros, and the ability to have the compiler halt on such issues would be helpful for diagnosis.
> >
> > Signed-off-by: Daniel Burgener <Daniel.Burgener@microsoft.com>
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

Applied.

Patch
diff mbox series

diff --git a/checkpolicy/checkmodule.8 b/checkpolicy/checkmodule.8
index e597d9d4..70e67f1e 100644
--- a/checkpolicy/checkmodule.8
+++ b/checkpolicy/checkmodule.8
@@ -28,6 +28,9 @@  module file.  This option is a development/debugging aid.
 .B \-C,\-\-cil
 Write CIL policy file rather than binary policy file.
 .TP
+.B \-E,\-\-werror
+Treat warnings as errors
+.TP
 .B \-h,\-\-help
 Print usage.
 .TP
diff --git a/checkpolicy/checkmodule.c b/checkpolicy/checkmodule.c
index c9efaf8b..ddf0700f 100644
--- a/checkpolicy/checkmodule.c
+++ b/checkpolicy/checkmodule.c
@@ -41,6 +41,7 @@  extern int optind;
 static sidtab_t sidtab;
 
 extern int mlspol;
+extern int werror;
 
 static int handle_unknown = SEPOL_DENY_UNKNOWN;
 static const char *txtfile = "policy.conf";
@@ -126,7 +127,7 @@  static int write_binary_policy(policydb_t * p, FILE *outfp)
 
 static __attribute__((__noreturn__)) void usage(const char *progname)
 {
-	printf("usage:  %s [-h] [-V] [-b] [-C] [-U handle_unknown] [-m] [-M] [-o FILE] [INPUT]\n", progname);
+	printf("usage:  %s [-h] [-V] [-b] [-C] [-E] [-U handle_unknown] [-m] [-M] [-o FILE] [INPUT]\n", progname);
 	printf("Build base and policy modules.\n");
 	printf("Options:\n");
 	printf("  INPUT      build module from INPUT (else read from \"%s\")\n",
@@ -134,6 +135,7 @@  static __attribute__((__noreturn__)) void usage(const char *progname)
 	printf("  -V         show policy versions created by this program\n");
 	printf("  -b         treat input as a binary policy file\n");
 	printf("  -C         output CIL policy instead of binary policy\n");
+	printf("  -E         treat warnings as errors\n");
 	printf("  -h         print usage\n");
 	printf("  -U OPTION  How to handle unknown classes and permissions\n");
 	printf("               deny: Deny unknown kernel checks\n");
@@ -162,10 +164,11 @@  int main(int argc, char **argv)
 		{"handle-unknown", required_argument, NULL, 'U'},
 		{"mls", no_argument, NULL, 'M'},
 		{"cil", no_argument, NULL, 'C'},
+		{"werror", no_argument, NULL, 'E'},
 		{NULL, 0, NULL, 0}
 	};
 
-	while ((ch = getopt_long(argc, argv, "ho:bVU:mMCc:", long_options, NULL)) != -1) {
+	while ((ch = getopt_long(argc, argv, "ho:bVEU:mMCc:", long_options, NULL)) != -1) {
 		switch (ch) {
 		case 'h':
 			usage(argv[0]);
@@ -180,6 +183,9 @@  int main(int argc, char **argv)
 		case 'V':
 			show_version = 1;
 			break;
+		case 'E':
+			werror = 1;
+			break;
 		case 'U':
 			if (!strcasecmp(optarg, "deny")) {
 				handle_unknown = DENY_UNKNOWN;
diff --git a/checkpolicy/checkpolicy.8 b/checkpolicy/checkpolicy.8
index 97e10ca7..3d7c7539 100644
--- a/checkpolicy/checkpolicy.8
+++ b/checkpolicy/checkpolicy.8
@@ -53,6 +53,9 @@  Specify the target platform (selinux or xen).
 .B \-O,\-\-optimize
 Optimize the final kernel policy (remove redundant rules).
 .TP
+.B \-E,\-\-werror
+Treat warnings as errors
+.TP
 .B \-V,\-\-version
 Show version information.
 .TP
diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
index 7c5b63f8..b6e2d43a 100644
--- a/checkpolicy/checkpolicy.c
+++ b/checkpolicy/checkpolicy.c
@@ -101,6 +101,7 @@  static sidtab_t sidtab;
 
 extern policydb_t *policydbp;
 extern int mlspol;
+extern int werror;
 
 static int handle_unknown = SEPOL_DENY_UNKNOWN;
 static const char *txtfile = "policy.conf";
@@ -113,7 +114,7 @@  static __attribute__((__noreturn__)) void usage(const char *progname)
 	printf
 	    ("usage:  %s [-b[F]] [-C] [-d] [-U handle_unknown (allow,deny,reject)] [-M] "
 	     "[-c policyvers (%d-%d)] [-o output_file|-] [-S] "
-	     "[-t target_platform (selinux,xen)] [-V] [input_file]\n",
+	     "[-t target_platform (selinux,xen)] [-E] [-V] [input_file]\n",
 	     progname, POLICYDB_VERSION_MIN, POLICYDB_VERSION_MAX);
 	exit(1);
 }
@@ -421,11 +422,12 @@  int main(int argc, char **argv)
 		{"conf",no_argument, NULL, 'F'},
 		{"sort", no_argument, NULL, 'S'},
 		{"optimize", no_argument, NULL, 'O'},
+		{"werror", no_argument, NULL, 'E'},
 		{"help", no_argument, NULL, 'h'},
 		{NULL, 0, NULL, 0}
 	};
 
-	while ((ch = getopt_long(argc, argv, "o:t:dbU:MCFSVc:Oh", long_options, NULL)) != -1) {
+	while ((ch = getopt_long(argc, argv, "o:t:dbU:MCFSVc:OEh", long_options, NULL)) != -1) {
 		switch (ch) {
 		case 'o':
 			outfile = optarg;
@@ -504,6 +506,9 @@  int main(int argc, char **argv)
 					policyvers = n;
 				break;
 			}
+		case 'E':
+			 werror = 1;
+			 break;
 		case 'h':
 		default:
 			usage(argv[0]);
diff --git a/checkpolicy/policy_scan.l b/checkpolicy/policy_scan.l
index e2f676e4..b90a7378 100644
--- a/checkpolicy/policy_scan.l
+++ b/checkpolicy/policy_scan.l
@@ -36,6 +36,7 @@  typedef int (* require_func_t)(void);
 
 static char linebuf[2][255];
 static unsigned int lno = 0;
+int werror = 0;
 int yywarn(const char *msg);
 
 void set_source_file(const char *name);
@@ -310,6 +311,9 @@  int yyerror(const char *msg)
 
 int yywarn(const char *msg)
 {
+	if (werror)
+		return yyerror(msg);
+
 	if (source_file[0])
 		fprintf(stderr, "%s:%ld:",
 			source_file, source_lineno);