[PATCH 1/6] libsemanage: make semanage_..._destroy return void

[PATCH 1/6] libsemanage: make semanage_..._destroy return void

[Nicolas Iooss]

semanage_module_info_destroy() always return 0 ("success") even though
some of its caller want to check its return value. For example commit
86e6ae67fd17 ("libsemanage: drop checks on
semanage_module_info_destroy() value") removed such a caller which was
buggy.

Discourage using the return value of semanage_..._destroy() functions by
making them return void.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsemanage/include/semanage/modules.h | 12 ++++--------
 libsemanage/src/modules.c              | 22 +++++++++-------------
 2 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/libsemanage/include/semanage/modules.h b/libsemanage/include/semanage/modules.h
index 4b93e54e7d21..a35de815abbf 100644
--- a/libsemanage/include/semanage/modules.h
+++ b/libsemanage/include/semanage/modules.h
@@ -79,12 +79,10 @@ int semanage_module_info_create(semanage_handle_t *sh,
 
 /* Frees the members of the module info struct.
  *
- * Returns 0 on success and -1 on failure.
- *
  * The caller should call free() on the struct.
  */
-int semanage_module_info_destroy(semanage_handle_t *handle,
-				 semanage_module_info_t *modinfo);
+void semanage_module_info_destroy(semanage_handle_t *handle,
+				  semanage_module_info_t *modinfo);
 
 /* Module Info Getters */
 
@@ -168,11 +166,9 @@ int semanage_module_key_create(semanage_handle_t *sh,
 
 /* Frees members of the @modkey, but not the struct. The caller should
  * call free() on struct.
- *
- * Returns 0 on success, and -1 on error.
  */
-int semanage_module_key_destroy(semanage_handle_t *sh,
-				semanage_module_key_t *modkey);
+void semanage_module_key_destroy(semanage_handle_t *sh,
+				 semanage_module_key_t *modkey);
 
 /* Module Key Getters */
 
diff --git a/libsemanage/src/modules.c b/libsemanage/src/modules.c
index 90c5e4917f97..7bbe3aa1e6db 100644
--- a/libsemanage/src/modules.c
+++ b/libsemanage/src/modules.c
@@ -281,19 +281,19 @@ int semanage_module_info_create(semanage_handle_t *sh,
 
 hidden_def(semanage_module_info_create)
 
-int semanage_module_info_destroy(semanage_handle_t *sh,
-				 semanage_module_info_t *modinfo)
+void semanage_module_info_destroy(semanage_handle_t *sh,
+				  semanage_module_info_t *modinfo)
 {
 	assert(sh);
 
 	if (!modinfo) {
-		return 0;
+		return;
 	}
 
 	free(modinfo->name);
 	free(modinfo->lang_ext);
 
-	return semanage_module_info_init(sh, modinfo);
+	semanage_module_info_init(sh, modinfo);
 }
 
 hidden_def(semanage_module_info_destroy)
@@ -323,11 +323,7 @@ int semanage_module_info_clone(semanage_handle_t *sh,
 	int status = 0;
 	int ret = 0;
 
-	ret = semanage_module_info_destroy(sh, target);
-	if (ret != 0) {
-		status = -1;
-		goto cleanup;
-	}
+	semanage_module_info_destroy(sh, target);
 
 	ret = semanage_module_info_set_priority(sh, target, source->priority);
 	if (ret != 0) {
@@ -685,18 +681,18 @@ int semanage_module_key_create(semanage_handle_t *sh,
 
 hidden_def(semanage_module_key_create)
 
-int semanage_module_key_destroy(semanage_handle_t *sh,
-				semanage_module_key_t *modkey)
+void semanage_module_key_destroy(semanage_handle_t *sh,
+				 semanage_module_key_t *modkey)
 {
 	assert(sh);
 
 	if (!modkey) {
-		return 0;
+		return;
 	}
 
 	free(modkey->name);
 
-	return semanage_module_key_init(sh, modkey);
+	semanage_module_key_init(sh, modkey);
 }
 
 hidden_def(semanage_module_key_destroy)
-- 
2.12.0

[PATCH 2/6] libsepol: cil: check cil_fill_list return value

[Nicolas Iooss]

cil_gen_default() and cil_gen_defaultrange() call cil_fill_list()
without checking its return value. If it failed, propagate the return
value to the caller.

This issue has been found using clang's static analyzer. It reported
"warning: Value stored to 'rc' is never read" four times.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_build_ast.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 8a19df480989..4b03dc35d408 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -5592,9 +5592,11 @@ int cil_gen_default(struct cil_tree_node *parse_current, struct cil_tree_node *a
 	if (parse_current->next->cl_head == NULL) {
 		cil_list_init(&def->class_strs, CIL_CLASS);
 		cil_list_append(def->class_strs, CIL_STRING, parse_current->next->data);
-		rc = SEPOL_OK;
 	} else {
 		rc = cil_fill_list(parse_current->next->cl_head, CIL_CLASS, &def->class_strs);
+		if (rc != SEPOL_OK) {
+			goto exit;
+		}
 	}
 
 	object = parse_current->next->next->data;
@@ -5657,9 +5659,11 @@ int cil_gen_defaultrange(struct cil_tree_node *parse_current, struct cil_tree_no
 	if (parse_current->next->cl_head == NULL) {
 		cil_list_init(&def->class_strs, CIL_CLASS);
 		cil_list_append(def->class_strs, CIL_STRING, parse_current->next->data);
-		rc = SEPOL_OK;
 	} else {
 		rc = cil_fill_list(parse_current->next->cl_head, CIL_CLASS, &def->class_strs);
+		if (rc != SEPOL_OK) {
+			goto exit;
+		}
 	}
 
 	object = parse_current->next->next->data;
-- 
2.12.0

[PATCH 3/6] libselinux: avoid calling strcmp() on a NULL pointer

[Nicolas Iooss]

When curcon is NULL, calling strcmp(curcon, newcon) produces an undefined
behavior. Avoid this by checking whether curcon is NULL beforehand.

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/src/selinux_restorecon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index eefd2cf83e32..a41fc48a82d8 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -664,7 +664,7 @@ static int restorecon_sb(const char *pathname, const struct stat *sb,
 		curcon = NULL;
 	}
 
-	if (strcmp(curcon, newcon) != 0) {
+	if (curcon == NULL || strcmp(curcon, newcon) != 0) {
 		if (!flags->set_specctx && curcon &&
 				    (is_context_customizable(curcon) > 0)) {
 			if (flags->verbose) {
-- 
2.12.0

[PATCH 4/6] libselinux: getsebool: always free names

[Nicolas Iooss]

When getsebool's main() fails to allocate memory for the boolean names,
it returns without freeing variables first, even though other errors do
this (with label "out").

This silences a warning reported by clang's static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/utils/getsebool.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/libselinux/utils/getsebool.c b/libselinux/utils/getsebool.c
index 3c6eba55b7a8..369945363535 100644
--- a/libselinux/utils/getsebool.c
+++ b/libselinux/utils/getsebool.c
@@ -15,7 +15,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *progname)
 int main(int argc, char **argv)
 {
 	int i, get_all = 0, rc = 0, active, pending, len = 0, opt;
-	char **names;
+	char **names = NULL;
 
 	while ((opt = getopt(argc, argv, "a")) > 0) {
 		switch (opt) {
@@ -55,7 +55,7 @@ int main(int argc, char **argv)
 		if (argc < 2)
 			usage(argv[0]);
 		len = argc - 1;
-		names = malloc(sizeof(char *) * len);
+		names = calloc(len, sizeof(char *));
 		if (!names) {
 			fprintf(stderr, "%s:  out of memory\n", argv[0]);
 			return 2;
@@ -65,7 +65,8 @@ int main(int argc, char **argv)
 			if (!names[i]) {
 				fprintf(stderr, "%s:  out of memory\n",
 					argv[0]);
-				return 2;
+				rc = 2;
+				goto out;
 			}
 		}
 	}
-- 
2.12.0

[PATCH 5/6] policycoreutils: newrole: do not free pw strings twice

[Nicolas Iooss]

In main(), if "extract_pw_data(&pw)" returns a failed value, it has
already freed pw.pw_name, pw.pw_dir and pw.pw_shell. These fields are
freed a second time in main's err_free label, which is incorrect. Work
around this by setting them to NULL after they are freed.

This issue has been found using clang's static analyzer.

While at it, make extract_pw_data() static.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 policycoreutils/newrole/newrole.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/policycoreutils/newrole/newrole.c b/policycoreutils/newrole/newrole.c
index faf937b94f6d..bed92e4e7494 100644
--- a/policycoreutils/newrole/newrole.c
+++ b/policycoreutils/newrole/newrole.c
@@ -412,7 +412,7 @@ static int verify_shell(const char *shell_name)
  * This function assigns malloc'd memory into the pw_copy struct.
  * Returns zero on success, non-zero otherwise
  */
-int extract_pw_data(struct passwd *pw_copy)
+static int extract_pw_data(struct passwd *pw_copy)
 {
 	uid_t uid;
 	struct passwd *pw;
@@ -456,6 +456,9 @@ int extract_pw_data(struct passwd *pw_copy)
 	free(pw->pw_name);
 	free(pw->pw_dir);
 	free(pw->pw_shell);
+	pw->pw_name = NULL;
+	pw->pw_dir = NULL;
+	pw->pw_shell = NULL;
 	return -1;
 }
 
-- 
2.12.0

[PATCH 6/6] policycoreutils: newrole: always initialize pw fields

[Nicolas Iooss]

In extract_pw_data(), if "getpwuid(uid)" fails, the function returns an
error value without initializing main's pw.pw_name. This leads main() to
call "free(pw.pw_name)" on an uninitialized value.

Use memset() to initialize structure pw in main().

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 policycoreutils/newrole/newrole.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/policycoreutils/newrole/newrole.c b/policycoreutils/newrole/newrole.c
index bed92e4e7494..077496d3b64d 100644
--- a/policycoreutils/newrole/newrole.c
+++ b/policycoreutils/newrole/newrole.c
@@ -1113,6 +1113,7 @@ int main(int argc, char *argv[])
 	 * malicious software), not to authorize the operation (which is covered
 	 * by policy).  Trusted path mechanism would be preferred.
 	 */
+	memset(&pw, 0, sizeof(pw));
 	if (extract_pw_data(&pw))
 		goto err_free;
 
-- 
2.12.0

Re: [PATCH 1/6] libsemanage: make semanage_..._destroy return void

[Stephen Smalley]

On Tue, 2017-04-11 at 23:45 +0200, Nicolas Iooss wrote:
> semanage_module_info_destroy() always return 0 ("success") even
> though
> some of its caller want to check its return value. For example commit
> 86e6ae67fd17 ("libsemanage: drop checks on
> semanage_module_info_destroy() value") removed such a caller which
> was
> buggy.
> 
> Discourage using the return value of semanage_..._destroy() functions
> by
> making them return void.

Sorry, my fault - didn't realize this was a public API when I suggested
it.  Can't break the library ABI.  We could use symbol versioning magic
to transition it over, but probably not worth it.

> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsemanage/include/semanage/modules.h | 12 ++++--------
>  libsemanage/src/modules.c              | 22 +++++++++-------------
>  2 files changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/libsemanage/include/semanage/modules.h
> b/libsemanage/include/semanage/modules.h
> index 4b93e54e7d21..a35de815abbf 100644
> --- a/libsemanage/include/semanage/modules.h
> +++ b/libsemanage/include/semanage/modules.h
> @@ -79,12 +79,10 @@ int semanage_module_info_create(semanage_handle_t
> *sh,
>  
>  /* Frees the members of the module info struct.
>   *
> - * Returns 0 on success and -1 on failure.
> - *
>   * The caller should call free() on the struct.
>   */
> -int semanage_module_info_destroy(semanage_handle_t *handle,
> -				 semanage_module_info_t *modinfo);
> +void semanage_module_info_destroy(semanage_handle_t *handle,
> +				  semanage_module_info_t *modinfo);
>  
>  /* Module Info Getters */
>  
> @@ -168,11 +166,9 @@ int semanage_module_key_create(semanage_handle_t
> *sh,
>  
>  /* Frees members of the @modkey, but not the struct. The caller
> should
>   * call free() on struct.
> - *
> - * Returns 0 on success, and -1 on error.
>   */
> -int semanage_module_key_destroy(semanage_handle_t *sh,
> -				semanage_module_key_t *modkey);
> +void semanage_module_key_destroy(semanage_handle_t *sh,
> +				 semanage_module_key_t *modkey);
>  
>  /* Module Key Getters */
>  
> diff --git a/libsemanage/src/modules.c b/libsemanage/src/modules.c
> index 90c5e4917f97..7bbe3aa1e6db 100644
> --- a/libsemanage/src/modules.c
> +++ b/libsemanage/src/modules.c
> @@ -281,19 +281,19 @@ int
> semanage_module_info_create(semanage_handle_t *sh,
>  
>  hidden_def(semanage_module_info_create)
>  
> -int semanage_module_info_destroy(semanage_handle_t *sh,
> -				 semanage_module_info_t *modinfo)
> +void semanage_module_info_destroy(semanage_handle_t *sh,
> +				  semanage_module_info_t *modinfo)
>  {
>  	assert(sh);
>  
>  	if (!modinfo) {
> -		return 0;
> +		return;
>  	}
>  
>  	free(modinfo->name);
>  	free(modinfo->lang_ext);
>  
> -	return semanage_module_info_init(sh, modinfo);
> +	semanage_module_info_init(sh, modinfo);
>  }
>  
>  hidden_def(semanage_module_info_destroy)
> @@ -323,11 +323,7 @@ int semanage_module_info_clone(semanage_handle_t
> *sh,
>  	int status = 0;
>  	int ret = 0;
>  
> -	ret = semanage_module_info_destroy(sh, target);
> -	if (ret != 0) {
> -		status = -1;
> -		goto cleanup;
> -	}
> +	semanage_module_info_destroy(sh, target);
>  
>  	ret = semanage_module_info_set_priority(sh, target, source-
> >priority);
>  	if (ret != 0) {
> @@ -685,18 +681,18 @@ int
> semanage_module_key_create(semanage_handle_t *sh,
>  
>  hidden_def(semanage_module_key_create)
>  
> -int semanage_module_key_destroy(semanage_handle_t *sh,
> -				semanage_module_key_t *modkey)
> +void semanage_module_key_destroy(semanage_handle_t *sh,
> +				 semanage_module_key_t *modkey)
>  {
>  	assert(sh);
>  
>  	if (!modkey) {
> -		return 0;
> +		return;
>  	}
>  
>  	free(modkey->name);
>  
> -	return semanage_module_key_init(sh, modkey);
> +	semanage_module_key_init(sh, modkey);
>  }
>  
>  hidden_def(semanage_module_key_destroy)

Re: [PATCH 6/6] policycoreutils: newrole: always initialize pw fields

[Stephen Smalley]

On Tue, 2017-04-11 at 23:46 +0200, Nicolas Iooss wrote:
> In extract_pw_data(), if "getpwuid(uid)" fails, the function returns
> an
> error value without initializing main's pw.pw_name. This leads main()
> to
> call "free(pw.pw_name)" on an uninitialized value.
> 
> Use memset() to initialize structure pw in main().
> 
> This issue has been found using clang's static analyzer.

Thanks, applied patches 2 through 6.

> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  policycoreutils/newrole/newrole.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/policycoreutils/newrole/newrole.c
> b/policycoreutils/newrole/newrole.c
> index bed92e4e7494..077496d3b64d 100644
> --- a/policycoreutils/newrole/newrole.c
> +++ b/policycoreutils/newrole/newrole.c
> @@ -1113,6 +1113,7 @@ int main(int argc, char *argv[])
>  	 * malicious software), not to authorize the operation
> (which is covered
>  	 * by policy).  Trusted path mechanism would be preferred.
>  	 */
> +	memset(&pw, 0, sizeof(pw));
>  	if (extract_pw_data(&pw))
>  		goto err_free;
>