diff mbox series

conf-parse.y checks (with style fixes removed)

Message ID alpine.LFD.2.21.1905181955170.29988@34-41-5D-CA-59-C7 (mailing list archive)
State Changes Requested
Headers show
Series conf-parse.y checks (with style fixes removed) | expand

Commit Message

Jokke Hämäläinen May 18, 2019, 4:57 p.m. UTC

Comments

Stephen Smalley May 24, 2019, 5:07 p.m. UTC | #1 
On 5/18/19 12:57 PM, Jokke Hämäläinen wrote:
> 
> diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y
> index b527e893..64b53c2d 100644
> --- a/libsemanage/src/conf-parse.y
> +++ b/libsemanage/src/conf-parse.y
> @@ -221,6 +221,9 @@ usepasswd: USEPASSWD '=' ARG {
>   
>   ignoredirs: IGNOREDIRS '=' ARG {
>   	current_conf->ignoredirs = strdup($3);
> +	if (current_conf->ignoredirs == NULL) {
> +		yyerror("could not allocate memory for current_conf->ignoredirs");

I wouldn't put the variable name/field name into the error message. 
"could not allocate memory for ignoredirs" would suffice.

> +	}
>   	free($3);
>    }
>   
> @@ -240,8 +243,8 @@ handle_unknown: HANDLE_UNKNOWN '=' ARG {
>   bzip_blocksize:  BZIP_BLOCKSIZE '=' ARG {
>   	int blocksize = atoi($3);
>   	free($3);
> -	if (blocksize > 9)
> -		yyerror("bzip-blocksize can only be in the range 0-9");
> +	if (blocksize < 1 || blocksize > 9)
> +		yyerror("bzip-blocksize can only be in the range 1-9");
>   	else
>   		current_conf->bzip_blocksize = blocksize;
>   }
> @@ -339,9 +342,18 @@ static int semanage_conf_init(semanage_conf_t * conf)
>   {
>   	conf->store_type = SEMANAGE_CON_DIRECT;
>   	conf->store_path = strdup(basename(selinux_policy_root()));
> +	if (conf->store_path == NULL) {
> +		return -1;
> +	}
>   	conf->ignoredirs = NULL;
>   	conf->store_root_path = strdup("/var/lib/selinux");
> +	if (conf->store_root_path == NULL) {
> +		return -1;
> +	}
>   	conf->compiler_directory_path = strdup("/usr/libexec/selinux/hll");
> +	if (conf->compiler_directory_path == NULL) {
> +		return -1;
> +	}
>   	conf->policyvers = sepol_policy_kern_vers_max();
>   	conf->target_platform = SEPOL_TARGET_SELINUX;
>   	conf->expand_check = 1;
> @@ -515,7 +527,10 @@ static int parse_module_store(char *arg)
>   			}
>   		}
>   	}
> -	return 0;
> +
> +	if (current_conf->store_path)
> +		return 0;
> +	return -3;
>   }
>   
>   static int parse_store_root_path(char *arg)
> @@ -526,6 +541,10 @@ static int parse_store_root_path(char *arg)
>   
>   	free(current_conf->store_root_path);
>   	current_conf->store_root_path = strdup(arg);
> +	if (current_conf->store_root_path == NULL) {
> +		return -2;

Not sure why we are using multiple error return codes here or what they 
mean.

> +	}
> +
>   	return 0;
>   }
>   
> @@ -536,6 +555,9 @@ static int parse_compiler_path(char *arg)
>   	}
>   	free(current_conf->compiler_directory_path);
>   	current_conf->compiler_directory_path = strdup(arg);
> +	if (current_conf->compiler_directory_path = NULL) {
> +		return -2;
> +	}
>   	return 0;
>   }
>   
>
selinux@kolttonen.fi May 24, 2019, 5:40 p.m. UTC | #2 
On Fri, 24 May 2019, Stephen Smalley wrote:

> I wouldn't put the variable name/field name into the error message. "could not
> allocate memory for ignoredirs" would suffice.

Agreed. I will fix it.
 
> > +	}
> >    	free($3);
> >    }
> >   @@ -240,8 +243,8 @@ handle_unknown: HANDLE_UNKNOWN '=' ARG {
> >   bzip_blocksize:  BZIP_BLOCKSIZE '=' ARG {
> >    int blocksize = atoi($3);
> >    free($3);
> > -	if (blocksize > 9)
> > -		yyerror("bzip-blocksize can only be in the range 0-9");
> > +	if (blocksize < 1 || blocksize > 9)
> > +		yyerror("bzip-blocksize can only be in the range 1-9");
> >    else
> >   		current_conf->bzip_blocksize = blocksize;
> >   }
> > @@ -339,9 +342,18 @@ static int semanage_conf_init(semanage_conf_t * conf)
> >   {
> >    conf->store_type = SEMANAGE_CON_DIRECT;
> >    conf->store_path = strdup(basename(selinux_policy_root()));
> > +	if (conf->store_path == NULL) {
> > +		return -1;
> > +	}
> >    conf->ignoredirs = NULL;
> >    conf->store_root_path = strdup("/var/lib/selinux");
> > +	if (conf->store_root_path == NULL) {
> > +		return -1;
> > +	}
> >   	conf->compiler_directory_path = strdup("/usr/libexec/selinux/hll");
> > +	if (conf->compiler_directory_path == NULL) {
> > +		return -1;
> > +	}
> >    conf->policyvers = sepol_policy_kern_vers_max();
> >    conf->target_platform = SEPOL_TARGET_SELINUX;
> >    conf->expand_check = 1;
> > @@ -515,7 +527,10 @@ static int parse_module_store(char *arg)
> >     	}
> >    	}
> >   	}
> > -	return 0;
> > +
> > +	if (current_conf->store_path)
> > +		return 0;
> > +	return -3;
> >   }
> >   
> >   static int parse_store_root_path(char *arg)
> > @@ -526,6 +541,10 @@ static int parse_store_root_path(char *arg)
> >   
> >    free(current_conf->store_root_path);
> >    current_conf->store_root_path = strdup(arg);
> > +	if (current_conf->store_root_path == NULL) {
> > +		return -2;
> 
> Not sure why we are using multiple error return codes here or what they mean.

I think some of the existing code in this file used "return -1" and 
"return -2", so I followed that choice just in case someone would be 
interested in inspecting why the functions failed. 

As of now, the callers do not care more specifically than checking for 
non-zero, but if someone would run the code in e.g. a debugger, it might 
be good to have separate return values for different errors - I am not 
sure, though.

If you want all errors to be -1, I can of course do that?

Regards,
Jokke Hämäläinen
diff mbox series

Patch

diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y
index b527e893..64b53c2d 100644
--- a/libsemanage/src/conf-parse.y
+++ b/libsemanage/src/conf-parse.y
@@ -221,6 +221,9 @@  usepasswd: USEPASSWD '=' ARG {
 
 ignoredirs: IGNOREDIRS '=' ARG {
 	current_conf->ignoredirs = strdup($3);
+	if (current_conf->ignoredirs == NULL) {
+		yyerror("could not allocate memory for current_conf->ignoredirs");
+	}
 	free($3);
  }
 
@@ -240,8 +243,8 @@  handle_unknown: HANDLE_UNKNOWN '=' ARG {
 bzip_blocksize:  BZIP_BLOCKSIZE '=' ARG {
 	int blocksize = atoi($3);
 	free($3);
-	if (blocksize > 9)
-		yyerror("bzip-blocksize can only be in the range 0-9");
+	if (blocksize < 1 || blocksize > 9)
+		yyerror("bzip-blocksize can only be in the range 1-9");
 	else
 		current_conf->bzip_blocksize = blocksize;
 }
@@ -339,9 +342,18 @@  static int semanage_conf_init(semanage_conf_t * conf)
 {
 	conf->store_type = SEMANAGE_CON_DIRECT;
 	conf->store_path = strdup(basename(selinux_policy_root()));
+	if (conf->store_path == NULL) {
+		return -1;
+	}
 	conf->ignoredirs = NULL;
 	conf->store_root_path = strdup("/var/lib/selinux");
+	if (conf->store_root_path == NULL) {
+		return -1;
+	}
 	conf->compiler_directory_path = strdup("/usr/libexec/selinux/hll");
+	if (conf->compiler_directory_path == NULL) {
+		return -1;
+	}
 	conf->policyvers = sepol_policy_kern_vers_max();
 	conf->target_platform = SEPOL_TARGET_SELINUX;
 	conf->expand_check = 1;
@@ -515,7 +527,10 @@  static int parse_module_store(char *arg)
 			}
 		}
 	}
-	return 0;
+
+	if (current_conf->store_path)
+		return 0;
+	return -3;
 }
 
 static int parse_store_root_path(char *arg)
@@ -526,6 +541,10 @@  static int parse_store_root_path(char *arg)
 
 	free(current_conf->store_root_path);
 	current_conf->store_root_path = strdup(arg);
+	if (current_conf->store_root_path == NULL) {
+		return -2;
+	}
+
 	return 0;
 }
 
@@ -536,6 +555,9 @@  static int parse_compiler_path(char *arg)
 	}
 	free(current_conf->compiler_directory_path);
 	current_conf->compiler_directory_path = strdup(arg);
+	if (current_conf->compiler_directory_path = NULL) {
+		return -2;
+	}
 	return 0;
 }