[PATCH 0/7] Fix some issues spotted by static analyzers

[PATCH 0/7] Fix some issues spotted by static analyzers

[Nicolas Iooss]

Hi,
As you may have noticed, I have been using clang's static analyzer for
a few months and submitted fixes for bugs that it found. There are also
many minor issues in the code (memory leaks, dead assignments, etc.)
which introduce much noise and make it harder to find real issues. For
example if a reported "dead variable assignment" is about the return
value of a function which would need to be checked and the wrong
variable is used in the check, this would be detected by the analyzer
but would be in the noise of other minor issues.

Therefore this patchset (and the ones that I would like to send in the
following weeks) is about reducing this noise.

I am starting with libsepol/src/module_to_cil.c and while cleaning up
the commits I have written in order to use clang's static analyzer, I
stumbled upon some other local commits I forgot to send, which fix more
important bugs (like a missing call to va_end(), in the last patch).
This explains why there are other files which are modified.

Here is the git shortlog:

Nicolas Iooss (7):
  libsepol: cil: silence clang analyzer false positive
  libsepol: do not leak memory if list_prepend fails
  libsepol: remove some dead assignments
  libsepol: do not call malloc with 0 byte
  libsepol: remove unused variable
  checkpolicy: destroy the class datum if it fails to initialize
  libsepol: destroy the copied va_list

 checkpolicy/module_compiler.c   |  1 +
 libsepol/cil/src/cil_tree.c     |  2 +-
 libsepol/src/kernel_to_common.c |  3 +++
 libsepol/src/module_to_cil.c    | 21 +++++++++++++--------
 4 files changed, 18 insertions(+), 9 deletions(-)

Cheers,
Nicolas

-- 
2.17.0

[PATCH 1/7] libsepol: cil: silence clang analyzer false positive

[Nicolas Iooss]

In cil_tree_print_expr(), "rc < 0" is equivalent to "rc != 0" but
clang's static analyzer does not know about this. Help it.

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

diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c
index 1b04fe68a302..b1cbda918565 100644
--- a/libsepol/cil/src/cil_tree.c
+++ b/libsepol/cil/src/cil_tree.c
@@ -512,7 +512,7 @@ void cil_tree_print_expr(struct cil_list *datum_expr, struct cil_list *str_expr)
 	} else {
 		rc = cil_expr_to_string(str_expr, &expr_str);
 	}
-	if (rc < 0) {
+	if (rc != SEPOL_OK) {
 		cil_log(CIL_INFO, "ERROR)");
 		return;
 	}
-- 
2.17.0

[PATCH 2/7] libsepol: do not leak memory if list_prepend fails

[Nicolas Iooss]

When list_prepend() returns an error, it always means it failed to
allocate some memory and does not hold any reference to its argument
data. This argument needs to be freed by the caller in order to prevent
a memory leak.

While reviewing list_prepend() callers, I spend quite some time
understanding why typealiases_gather_map() does not need to strdup(key)
or free(key) when calling list_prepend(..., key) even though "key" comes
from pdb->p_types.table: because typealias_list_destroy() does not free
the inserted items. Add a comment to make this clearer in the code.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/module_to_cil.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index c6f1659c84ef..ba8311ea7c75 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -298,6 +298,8 @@ static int roles_gather_map(char *key, void *data, void *args)
 	role_node->role = role;
 
 	rc = list_prepend((struct list *)args, role_node);
+	if (rc != 0)
+		free(role_node);
 	return rc;
 }
 
@@ -344,6 +346,11 @@ static int typealiases_gather_map(char *key, void *data, void *arg)
 					goto exit;
 				}
 			}
+			/* As typealias_lists[scope_id] does not hold the
+			 * ownership of its items (typealias_list_destroy does
+			 * not free the list items), "key" does not need to be
+			 * strdup'ed before it is inserted in the list.
+			 */
 			list_prepend(typealias_lists[scope_id], key);
 		}
 	}
-- 
2.17.0

[PATCH 3/7] libsepol: remove some dead assignments

[Nicolas Iooss]

clang's static analyzer warns about dead assignments to local variables.
In module_to_cil.c, there are some which are quite straightforward to
review. Remove them.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/module_to_cil.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index ba8311ea7c75..310cf1a7b1c1 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -1099,7 +1099,6 @@ static int roletype_role_in_ancestor_to_cil(struct policydb *pdb, struct stack *
 		goto exit;
 	}
 
-	curr = role_list->head;
 	for (curr = role_list->head; curr != NULL; curr = curr->next) {
 		role_node = curr->data;
 		if (!is_id_in_ancestor_scope(pdb, decl_stack, role_node->role_name, SYM_ROLES)) {
@@ -1291,7 +1290,6 @@ static int cond_expr_to_cil(int indent, struct policydb *pdb, struct cond_expr *
 				rc = -1;
 				goto exit;
 			}
-			num_params = 0;
 		} else {
 			switch(curr->expr_type) {
 			case COND_NOT:	op = "not";	break;
@@ -1831,8 +1829,6 @@ static int constraint_expr_to_string(struct policydb *pdb, struct constraint_exp
 				free(names);
 				names = NULL;
 			}
-
-			num_params = 0;
 		} else {
 			switch (expr->expr_type) {
 			case CEXPR_NOT: op = "not"; break;
-- 
2.17.0

[PATCH 4/7] libsepol: do not call malloc with 0 byte

[Nicolas Iooss]

clang's static analyzer reports that ebitmap_to_names() can call
malloc(0) when the bitmap is empty. If malloc() returns NULL, this
triggers a misleading "Out of memory" error.

Work around this by treating empty bitmaps as appropriate.

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

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 310cf1a7b1c1..56887366707a 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -1009,6 +1009,12 @@ static int ebitmap_to_names(struct ebitmap *map, char **vals_to_names, char ***n
 		}
 	}
 
+	if (!num) {
+		*names = NULL;
+		*num_names = 0;
+		goto exit;
+	}
+
 	name_arr = malloc(sizeof(*name_arr) * num);
 	if (name_arr == NULL) {
 		log_err("Out of memory");
-- 
2.17.0

[PATCH 5/7] libsepol: remove unused variable

[Nicolas Iooss]

sepol_ppfile_to_module_package() does not use its variable "FILE *f =
NULL;" but to fclose() it. This variable has been unneeded since the
introduction of function ppfile_to_module_package() in commit
893851c0a146 ("policycoreutils: add a HLL compiler to convert policy
packages (.pp) to CIL").

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/module_to_cil.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 56887366707a..dcf6ebb10b60 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -4232,7 +4232,6 @@ exit:
 int sepol_ppfile_to_module_package(FILE *fp, struct sepol_module_package **mod_pkg)
 {
 	int rc = -1;
-	FILE *f = NULL;
 	struct sepol_policy_file *pf = NULL;
 	struct sepol_module_package *pkg = NULL;
 	char *data = NULL;
@@ -4284,9 +4283,6 @@ exit:
 	free(data);
 
 	sepol_policy_file_free(pf);
-	if (f != NULL) {
-		fclose(f);
-	}
 
 	if (rc != 0) {
 		sepol_module_package_free(pkg);
-- 
2.17.0

[PATCH 6/7] checkpolicy: destroy the class datum if it fails to initialize

[Nicolas Iooss]

require_class() allocate memory for its variable "class_datum_t *datum"
and calls symtab_init(&datum->permissions, PERM_SYMTAB_SIZE). If this
second call fails, datum is not freed.

Fix this memory leak.

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

diff --git a/checkpolicy/module_compiler.c b/checkpolicy/module_compiler.c
index 155702f2731b..ada7cb2ae9c7 100644
--- a/checkpolicy/module_compiler.c
+++ b/checkpolicy/module_compiler.c
@@ -802,6 +802,7 @@ int require_class(int pass)
 	if ((datum = calloc(1, sizeof(*datum))) == NULL ||
 	    symtab_init(&datum->permissions, PERM_SYMTAB_SIZE)) {
 		yyerror("Out of memory!");
+		class_datum_destroy(datum);
 		return -1;
 	}
 	ret =
-- 
2.17.0

[PATCH 7/7] libsepol: destroy the copied va_list

[Nicolas Iooss]

va_copy()'s manpage [1] states:

    Each invocation of va_copy() must be matched by a corresponding
    invocation of va_end() in the same function.

create_str_helper() is using va_copy() without va_end(). Add the missing
call.

[1] https://linux.die.net/man/3/va_copy

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/kernel_to_common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libsepol/src/kernel_to_common.c b/libsepol/src/kernel_to_common.c
index 342bc3c91df9..7c5699c52c95 100644
--- a/libsepol/src/kernel_to_common.c
+++ b/libsepol/src/kernel_to_common.c
@@ -80,10 +80,13 @@ static char *create_str_helper(const char *fmt, int num, va_list vargs)
 		goto exit;
 	}
 
+	va_end(vargs2);
+
 	return str;
 
 exit:
 	free(str);
+	va_end(vargs2);
 	return NULL;
 }
 
-- 
2.17.0

Re: [PATCH 0/7] Fix some issues spotted by static analyzers

[Jason Zaman]

On Sat, May 26, 2018 at 08:42:06PM +0200, Nicolas Iooss wrote:
> Hi,
> As you may have noticed, I have been using clang's static analyzer for
> a few months and submitted fixes for bugs that it found. There are also
> many minor issues in the code (memory leaks, dead assignments, etc.)
> which introduce much noise and make it harder to find real issues. For
> example if a reported "dead variable assignment" is about the return
> value of a function which would need to be checked and the wrong
> variable is used in the check, this would be detected by the analyzer
> but would be in the noise of other minor issues.
> 
> Therefore this patchset (and the ones that I would like to send in the
> following weeks) is about reducing this noise.
> 
> I am starting with libsepol/src/module_to_cil.c and while cleaning up
> the commits I have written in order to use clang's static analyzer, I
> stumbled upon some other local commits I forgot to send, which fix more
> important bugs (like a missing call to va_end(), in the last patch).
> This explains why there are other files which are modified.
> 
> Here is the git shortlog:
> 
> Nicolas Iooss (7):
>   libsepol: cil: silence clang analyzer false positive
>   libsepol: do not leak memory if list_prepend fails
>   libsepol: remove some dead assignments
>   libsepol: do not call malloc with 0 byte
>   libsepol: remove unused variable
>   checkpolicy: destroy the class datum if it fails to initialize
>   libsepol: destroy the copied va_list
> 
>  checkpolicy/module_compiler.c   |  1 +
>  libsepol/cil/src/cil_tree.c     |  2 +-
>  libsepol/src/kernel_to_common.c |  3 +++
>  libsepol/src/module_to_cil.c    | 21 +++++++++++++--------
>  4 files changed, 18 insertions(+), 9 deletions(-)

These all look good to me. I didnt compile test since your CI has
already done that many many times.

For the whole series:
Acked-by: Jason Zaman <jason@perfinion.com>

-- Jason

> 
> Cheers,
> Nicolas
> 
> -- 
> 2.17.0
> 
> 
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.

Re: [PATCH 7/7] libsepol: destroy the copied va_list

[William Roberts]

ack

On Sat, May 26, 2018 at 11:42 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> va_copy()'s manpage [1] states:
>
>     Each invocation of va_copy() must be matched by a corresponding
>     invocation of va_end() in the same function.
>
> create_str_helper() is using va_copy() without va_end(). Add the missing
> call.
>
> [1] https://linux.die.net/man/3/va_copy
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/src/kernel_to_common.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libsepol/src/kernel_to_common.c b/libsepol/src/kernel_to_common.c
> index 342bc3c91df9..7c5699c52c95 100644
> --- a/libsepol/src/kernel_to_common.c
> +++ b/libsepol/src/kernel_to_common.c
> @@ -80,10 +80,13 @@ static char *create_str_helper(const char *fmt, int num, va_list vargs)
>                 goto exit;
>         }
>
> +       va_end(vargs2);
> +
>         return str;
>
>  exit:
>         free(str);
> +       va_end(vargs2);
>         return NULL;
>  }
>
> --
> 2.17.0
>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.

Re: [PATCH 0/7] Fix some issues spotted by static analyzers

[Nicolas Iooss]

On Mon, May 28, 2018 at 6:42 AM, Jason Zaman <jason@perfinion.com> wrote:
> On Sat, May 26, 2018 at 08:42:06PM +0200, Nicolas Iooss wrote:
>> Hi,
>> As you may have noticed, I have been using clang's static analyzer for
>> a few months and submitted fixes for bugs that it found. There are also
>> many minor issues in the code (memory leaks, dead assignments, etc.)
>> which introduce much noise and make it harder to find real issues. For
>> example if a reported "dead variable assignment" is about the return
>> value of a function which would need to be checked and the wrong
>> variable is used in the check, this would be detected by the analyzer
>> but would be in the noise of other minor issues.
>>
>> Therefore this patchset (and the ones that I would like to send in the
>> following weeks) is about reducing this noise.
>>
>> I am starting with libsepol/src/module_to_cil.c and while cleaning up
>> the commits I have written in order to use clang's static analyzer, I
>> stumbled upon some other local commits I forgot to send, which fix more
>> important bugs (like a missing call to va_end(), in the last patch).
>> This explains why there are other files which are modified.
>>
>> Here is the git shortlog:
>>
>> Nicolas Iooss (7):
>>   libsepol: cil: silence clang analyzer false positive
>>   libsepol: do not leak memory if list_prepend fails
>>   libsepol: remove some dead assignments
>>   libsepol: do not call malloc with 0 byte
>>   libsepol: remove unused variable
>>   checkpolicy: destroy the class datum if it fails to initialize
>>   libsepol: destroy the copied va_list
>>
>>  checkpolicy/module_compiler.c   |  1 +
>>  libsepol/cil/src/cil_tree.c     |  2 +-
>>  libsepol/src/kernel_to_common.c |  3 +++
>>  libsepol/src/module_to_cil.c    | 21 +++++++++++++--------
>>  4 files changed, 18 insertions(+), 9 deletions(-)
>
> These all look good to me. I didnt compile test since your CI has
> already done that many many times.
>
> For the whole series:
> Acked-by: Jason Zaman <jason@perfinion.com>

Thanks! I applied the commits.

Nicolas