[PATCH 0/9] Fix issues found by static analyzers

[PATCH 0/9] Fix issues found by static analyzers

[Nicolas Iooss]

Hi,

In August I ran several free static analyzers in order to find new
issues in the userspace code. I used:

* clang's static analyzer (with scripts/run-scan-build)
* https://lgtm.com/ (I wrote a simple configuration file to analyze the
  C code, the results are available on
  https://lgtm.com/projects/g/fishilico/selinux-for-lgtm/ )
* Facebook's Infer (https://fbinfer.com/, I wrote a script to invoke it
  like clang's static analyzer)

Each analyzer gave many results, with several true bugs in them. Here
are patches that fix some of them. I will post the configuration files
and scripts I used separately.

Thanks,
Nicolas

Nicolas Iooss (9):
  semodule-utils: fix comparison with argc
  libsepol/cil: help static analyzers by aborting when an allocation
    fails
  libsepol: do not dereference a failed allocated pointer
  libsepol: do not dereference scope if it can be NULL
  libsepol: reset *p to NULL if sepol_module_package_create fails
  libsepol/cil: do not dereference perm_value_to_cil when it has not
    been allocated
  python/chcat: remove unnecessary assignment
  python/sepolicy: remove unnecessary pass statement
  libsepol/tests: do not dereference a NULL pointer

 libsepol/cil/src/cil_binary.c                        | 10 ++++++----
 libsepol/cil/src/cil_mem.c                           |  5 +++++
 libsepol/src/avrule_block.c                          |  3 ++-
 libsepol/src/kernel_to_cil.c                         |  8 +++++---
 libsepol/src/kernel_to_conf.c                        |  8 +++++---
 libsepol/src/module.c                                |  4 +++-
 libsepol/tests/test-common.c                         | 11 +++++++----
 python/chcat/chcat                                   |  1 -
 python/sepolicy/sepolicy/__init__.py                 |  1 -
 semodule-utils/semodule_package/semodule_unpackage.c |  2 +-
 10 files changed, 34 insertions(+), 19 deletions(-)

-- 
2.22.0

[PATCH 1/9] semodule-utils: fix comparison with argc

[Nicolas Iooss]

In order for argv[3] to be used, argc needs to be at least 4, not 3.

This bug was found using lgtm.com analyzer:
https://lgtm.com/projects/g/fishilico/selinux-for-lgtm/snapshot/8c1b2658f80392ff8b3532c6bd5d0cefac8afb30/files/semodule-utils/semodule_package/semodule_unpackage.c?sort=name&dir=ASC&mode=heatmap#xb1ce80b43260d34c:1

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

diff --git a/semodule-utils/semodule_package/semodule_unpackage.c b/semodule-utils/semodule_package/semodule_unpackage.c
index c9124c0f5ac1..b8c4fbcec434 100644
--- a/semodule-utils/semodule_package/semodule_unpackage.c
+++ b/semodule-utils/semodule_package/semodule_unpackage.c
@@ -55,7 +55,7 @@ int main(int argc, char **argv)
 
 	ppfile = argv[1];
 	modfile = argv[2];
-	if (argc >= 3)
+	if (argc >= 4)
 		fcfile = argv[3];
 
 	if (file_to_policy_file(ppfile, &in, "r"))
-- 
2.22.0

[PATCH 2/9] libsepol/cil: help static analyzers by aborting when an allocation fails

[Nicolas Iooss]

When allocating memory with cil_* helpers, if malloc/calloc/realloc/...
failed, (*cil_mem_error_handler)() is called. Implementations of this
function are expected not to return to the caller, and the default one
calls exit(1) to ensure this. In order for static analyzers to find out
that cil_malloc/cil_realloc/... never returns a NULL pointer when
failing to allocate some memory, introduce a call to abort().

This decreases the number of false positive warnings about null pointer
dereferences reported by Infer static analyzer.

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

diff --git a/libsepol/cil/src/cil_mem.c b/libsepol/cil/src/cil_mem.c
index 12c59be21914..885431d8a8fd 100644
--- a/libsepol/cil/src/cil_mem.c
+++ b/libsepol/cil/src/cil_mem.c
@@ -55,6 +55,7 @@ void *cil_malloc(size_t size)
 			return NULL;
 		}
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return mem;
@@ -65,6 +66,7 @@ void *cil_calloc(size_t num_elements, size_t element_size)
 	void *mem = calloc(num_elements, element_size);
 	if (mem == NULL){
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return mem;
@@ -78,6 +80,7 @@ void *cil_realloc(void *ptr, size_t size)
 			return NULL;
 		}
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return mem;
@@ -95,6 +98,7 @@ char *cil_strdup(const char *str)
 	mem = strdup(str);
 	if (mem == NULL) {
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return mem;
@@ -111,6 +115,7 @@ __attribute__ ((format (printf, 2, 3))) int cil_asprintf(char **strp, const char
 
 	if (rc == -1) {
 		(*cil_mem_error_handler)();
+		abort();
 	}
 
 	return rc;
-- 
2.22.0

[PATCH 3/9] libsepol: do not dereference a failed allocated pointer

[Nicolas Iooss]

When strs_stack_init(&stack) fails to allocate memory and stack is still
NULL, it should not be dereferenced with strs_stack_pop(stack).

This issue has been found using Infer static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/kernel_to_cil.c  | 8 +++++---
 libsepol/src/kernel_to_conf.c | 8 +++++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index 320af37b2bc8..9fcc254707ba 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -108,10 +108,12 @@ static char *cond_expr_to_str(struct policydb *pdb, struct cond_expr *expr)
 	return str;
 
 exit:
-	while ((new_val = strs_stack_pop(stack)) != NULL) {
-		free(new_val);
+	if (stack) {
+		while ((new_val = strs_stack_pop(stack)) != NULL) {
+			free(new_val);
+		}
+		strs_stack_destroy(&stack);
 	}
-	strs_stack_destroy(&stack);
 
 	return NULL;
 }
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 930bafabdd4b..2c8da49a11ab 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -106,10 +106,12 @@ static char *cond_expr_to_str(struct policydb *pdb, struct cond_expr *expr)
 	return str;
 
 exit:
-	while ((new_val = strs_stack_pop(stack)) != NULL) {
-		free(new_val);
+	if (stack != NULL) {
+		while ((new_val = strs_stack_pop(stack)) != NULL) {
+			free(new_val);
+		}
+		strs_stack_destroy(&stack);
 	}
-	strs_stack_destroy(&stack);
 
 	return NULL;
 }
-- 
2.22.0

[PATCH 4/9] libsepol: do not dereference scope if it can be NULL

[Nicolas Iooss]

Doing this looks wrong:

    len = scope->decl_ids_len;
    if (scope == NULL) {
        /* ... */

Move the dereferencing of scope after the NULL check.

This issue has been found using Infer static analyzer.

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

diff --git a/libsepol/src/avrule_block.c b/libsepol/src/avrule_block.c
index 5a873af4a864..a9832d0d118f 100644
--- a/libsepol/src/avrule_block.c
+++ b/libsepol/src/avrule_block.c
@@ -157,7 +157,7 @@ int is_id_enabled(char *id, policydb_t * p, int symbol_table)
 	scope_datum_t *scope =
 	    (scope_datum_t *) hashtab_search(p->scope[symbol_table].table, id);
 	avrule_decl_t *decl;
-	uint32_t len = scope->decl_ids_len;
+	uint32_t len;
 
 	if (scope == NULL) {
 		return 0;
@@ -166,6 +166,7 @@ int is_id_enabled(char *id, policydb_t * p, int symbol_table)
 		return 0;
 	}
 
+	len = scope->decl_ids_len;
 	if (len < 1) {
 		return 0;
 	}
-- 
2.22.0

[PATCH 5/9] libsepol: reset *p to NULL if sepol_module_package_create fails

[Nicolas Iooss]

semodule-utils/semodule_link/semodule_link.c contains:

    static sepol_module_package_t *load_module(char *filename)
    {
        /* ... */
        if (sepol_module_package_create(&p)) {
            /* ... */
            goto bad;

    /* ... */
    bad:
        sepol_module_package_free(p);

When sepol_module_package_create() fails while having successfully
allocated p, it currently frees p without setting it back to NULL. This
causes a use-after-free in load_module().

Prevent this use-after-free by setting sepol_module_package_create's
argument back to NULL when an error happens.

This issue has been found using Infer static analyzer.

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

diff --git a/libsepol/src/module.c b/libsepol/src/module.c
index 219355f30d27..3b8a0a59ca68 100644
--- a/libsepol/src/module.c
+++ b/libsepol/src/module.c
@@ -124,8 +124,10 @@ int sepol_module_package_create(sepol_module_package_t ** p)
 		return -1;
 
 	rc = module_package_init(*p);
-	if (rc < 0)
+	if (rc < 0) {
 		free(*p);
+		*p = NULL;
+	}
 
 	return rc;
 }
-- 
2.22.0

[PATCH 6/9] libsepol/cil: do not dereference perm_value_to_cil when it has not been allocated

[Nicolas Iooss]

When one of the first allocations of cil_binary_create_allocated_pdb()
fails, the exit label dereferences the items of array perm_value_to_cil
even though it could be still NULL.

This issue has been found using clang's static analyzer:
https://327-118970575-gh.circle-artifacts.com/0/output-scan-build/2019-08-05-203459-6149-1/report-febf85.html#EndPath

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

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 77ffc36f20ff..c882d5b74226 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -5043,11 +5043,13 @@ exit:
 	hashtab_destroy(avrulex_ioctl_table);
 	free(type_value_to_cil);
 	free(class_value_to_cil);
-	/* Range is because libsepol values start at 1. */
-	for (i=1; i < db->num_classes+1; i++) {
-		free(perm_value_to_cil[i]);
+	if (perm_value_to_cil != NULL) {
+		/* Range is because libsepol values start at 1. */
+		for (i=1; i < db->num_classes+1; i++) {
+			free(perm_value_to_cil[i]);
+		}
+		free(perm_value_to_cil);
 	}
-	free(perm_value_to_cil);
 	cil_list_destroy(&neverallows, CIL_FALSE);
 
 	return rc;
-- 
2.22.0

[PATCH 7/9] python/chcat: remove unnecessary assignment

[Nicolas Iooss]

chcat_add() defines variable cmd twice before calling
subprocess.check_call(cmd, ...). Remove the first definition.

This bug was found using lgtm.com analyzer:
https://lgtm.com/projects/g/SELinuxProject/selinux/snapshot/eac5e661ca7300800000496fe13985286af70c6d/files/python/chcat/chcat?sort=name&dir=ASC&mode=heatmap#L118

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 python/chcat/chcat | 1 -
 1 file changed, 1 deletion(-)

diff --git a/python/chcat/chcat b/python/chcat/chcat
index ba398684376d..fdd2e46ee3f9 100755
--- a/python/chcat/chcat
+++ b/python/chcat/chcat
@@ -115,7 +115,6 @@ def chcat_add(orig, newcat, objects, login_ind):
     errors = 0
     sensitivity = newcat[0]
     cat = newcat[1]
-    cmd = 'chcon -l %s' % sensitivity
     for f in objects:
         (rc, c) = selinux.getfilecon(f)
         con = c.split(":")[3:]
-- 
2.22.0

[PATCH 8/9] python/sepolicy: remove unnecessary pass statement

[Nicolas Iooss]

This issue has been found using lgtm.com analyzer:
https://lgtm.com/projects/g/SELinuxProject/selinux/snapshot/eac5e661ca7300800000496fe13985286af70c6d/files/python/sepolicy/sepolicy/__init__.py?sort=name&dir=ASC&mode=heatmap#x9f8225117f52fb01:1

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 python/sepolicy/sepolicy/__init__.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/python/sepolicy/sepolicy/__init__.py b/python/sepolicy/sepolicy/__init__.py
index 6aed31bddf1e..e4540977d042 100644
--- a/python/sepolicy/sepolicy/__init__.py
+++ b/python/sepolicy/sepolicy/__init__.py
@@ -539,7 +539,6 @@ def find_file(reg):
             path += "/"
     except IndexError:
         print("try failed got an IndexError")
-        pass
 
     try:
         pat = re.compile(r"%s$" % reg)
-- 
2.22.0

[PATCH 9/9] libsepol/tests: do not dereference a NULL pointer

[Nicolas Iooss]

In test_attr_types, the pointer decl is allowed to be NULL in the
beginning, but is dereferenced to produce a helpful message right before
a CU_ASSERT_FATAL. Make this derefence not happen if the pointer is
NULL.

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

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/tests/test-common.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/libsepol/tests/test-common.c b/libsepol/tests/test-common.c
index e6619ed1d152..1d902880fe2e 100644
--- a/libsepol/tests/test-common.c
+++ b/libsepol/tests/test-common.c
@@ -228,13 +228,16 @@ void test_attr_types(policydb_t * p, const char *id, avrule_decl_t * decl, const
 	unsigned int i;
 	type_datum_t *attr;
 
-	if (decl)
+	if (decl) {
 		attr = hashtab_search(decl->p_types.table, id);
-	else
+		if (attr == NULL)
+			printf("could not find attr %s in decl %d\n", id, decl->decl_id);
+	} else {
 		attr = hashtab_search(p->p_types.table, id);
+		if (attr == NULL)
+			printf("could not find attr %s in policy\n", id);
+	}
 
-	if (attr == NULL)
-		printf("could not find attr %s in decl %d\n", id, decl->decl_id);
 	CU_ASSERT_FATAL(attr != NULL);
 	CU_ASSERT(attr->flavor == TYPE_ATTRIB);
 	CU_ASSERT(attr->primary == 1);
-- 
2.22.0

Re: [Non-DoD Source] [PATCH 2/9] libsepol/cil: help static analyzers by aborting when an allocation fails

[jwcart2]

On 9/1/19 2:06 PM, Nicolas Iooss wrote:
> When allocating memory with cil_* helpers, if malloc/calloc/realloc/...
> failed, (*cil_mem_error_handler)() is called. Implementations of this
> function are expected not to return to the caller, and the default one
> calls exit(1) to ensure this. In order for static analyzers to find out
> that cil_malloc/cil_realloc/... never returns a NULL pointer when
> failing to allocate some memory, introduce a call to abort().
> 
> This decreases the number of false positive warnings about null pointer
> dereferences reported by Infer static analyzer.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>   libsepol/cil/src/cil_mem.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/libsepol/cil/src/cil_mem.c b/libsepol/cil/src/cil_mem.c
> index 12c59be21914..885431d8a8fd 100644
> --- a/libsepol/cil/src/cil_mem.c
> +++ b/libsepol/cil/src/cil_mem.c
> @@ -55,6 +55,7 @@ void *cil_malloc(size_t size)
>   			return NULL;
>   		}
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
> 

I am going to have to think about this one.

I think the real answer might be to rip out the (*cil_mem_error_handler)() 
statements and replace them with what is in the default handler. I notice now 
that even though there is a cil_set_mem_error_handler() function, it is not used 
anywhere.

>   	return mem;
> @@ -65,6 +66,7 @@ void *cil_calloc(size_t num_elements, size_t element_size)
>   	void *mem = calloc(num_elements, element_size);
>   	if (mem == NULL){
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
>   
>   	return mem;
> @@ -78,6 +80,7 @@ void *cil_realloc(void *ptr, size_t size)
>   			return NULL;
>   		}
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
>   
>   	return mem;
> @@ -95,6 +98,7 @@ char *cil_strdup(const char *str)
>   	mem = strdup(str);
>   	if (mem == NULL) {
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
>   
>   	return mem;
> @@ -111,6 +115,7 @@ __attribute__ ((format (printf, 2, 3))) int cil_asprintf(char **strp, const char
>   
>   	if (rc == -1) {
>   		(*cil_mem_error_handler)();
> +		abort();
>   	}
>   
>   	return rc;
> 


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

Re: [Non-DoD Source] [PATCH 3/9] libsepol: do not dereference a failed allocated pointer

[jwcart2]

On 9/1/19 2:06 PM, Nicolas Iooss wrote:
> When strs_stack_init(&stack) fails to allocate memory and stack is still
> NULL, it should not be dereferenced with strs_stack_pop(stack).
> 
> This issue has been found using Infer static analyzer.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>   libsepol/src/kernel_to_cil.c  | 8 +++++---
>   libsepol/src/kernel_to_conf.c | 8 +++++---
>   2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index 320af37b2bc8..9fcc254707ba 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -108,10 +108,12 @@ static char *cond_expr_to_str(struct policydb *pdb, struct cond_expr *expr)
>   	return str;
>   
>   exit:
> -	while ((new_val = strs_stack_pop(stack)) != NULL) {
> -		free(new_val);
> +	if (stack) {
> +		while ((new_val = strs_stack_pop(stack)) != NULL) {
> +			free(new_val);
> +		}
> +		strs_stack_destroy(&stack);
>   	}
> -	strs_stack_destroy(&stack);
>   
>   	return NULL;
>   }

constraint_expr_to_str() has the same problem.

> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
> index 930bafabdd4b..2c8da49a11ab 100644
> --- a/libsepol/src/kernel_to_conf.c
> +++ b/libsepol/src/kernel_to_conf.c
> @@ -106,10 +106,12 @@ static char *cond_expr_to_str(struct policydb *pdb, struct cond_expr *expr)
>   	return str;
>   
>   exit:
> -	while ((new_val = strs_stack_pop(stack)) != NULL) {
> -		free(new_val);
> +	if (stack != NULL) {
> +		while ((new_val = strs_stack_pop(stack)) != NULL) {
> +			free(new_val);
> +		}
> +		strs_stack_destroy(&stack);
>   	}
> -	strs_stack_destroy(&stack);
>   
>   	return NULL;
>   }
> 

Same comment here. constraint_expr_to_str() also needs to be fixed.

-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

Re: [Non-DoD Source] [PATCH 3/9] libsepol: do not dereference a failed allocated pointer

[jwcart2]

On 9/10/19 2:52 PM, jwcart2 wrote:
> On 9/1/19 2:06 PM, Nicolas Iooss wrote:
>> When strs_stack_init(&stack) fails to allocate memory and stack is still
>> NULL, it should not be dereferenced with strs_stack_pop(stack).
>>
>> This issue has been found using Infer static analyzer.
>>
>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>> ---
>>   libsepol/src/kernel_to_cil.c  | 8 +++++---
>>   libsepol/src/kernel_to_conf.c | 8 +++++---
>>   2 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
>> index 320af37b2bc8..9fcc254707ba 100644
>> --- a/libsepol/src/kernel_to_cil.c
>> +++ b/libsepol/src/kernel_to_cil.c
>> @@ -108,10 +108,12 @@ static char *cond_expr_to_str(struct policydb *pdb, 
>> struct cond_expr *expr)
>>       return str;
>>   exit:
>> -    while ((new_val = strs_stack_pop(stack)) != NULL) {
>> -        free(new_val);
>> +    if (stack) {
>> +        while ((new_val = strs_stack_pop(stack)) != NULL) {
>> +            free(new_val);
>> +        }
>> +        strs_stack_destroy(&stack);
>>       }
>> -    strs_stack_destroy(&stack);
>>       return NULL;
>>   }
> 
> constraint_expr_to_str() has the same problem.
> 
>> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
>> index 930bafabdd4b..2c8da49a11ab 100644
>> --- a/libsepol/src/kernel_to_conf.c
>> +++ b/libsepol/src/kernel_to_conf.c
>> @@ -106,10 +106,12 @@ static char *cond_expr_to_str(struct policydb *pdb, 
>> struct cond_expr *expr)
>>       return str;
>>   exit:
>> -    while ((new_val = strs_stack_pop(stack)) != NULL) {
>> -        free(new_val);
>> +    if (stack != NULL) {

I forgot to mention that you should just use "if (stack) {" like above so the 
syntax is consistent.

Thanks,
Jim

>> +        while ((new_val = strs_stack_pop(stack)) != NULL) {
>> +            free(new_val);
>> +        }
>> +        strs_stack_destroy(&stack);
>>       }
>> -    strs_stack_destroy(&stack);
>>       return NULL;
>>   }
>>
> 
> Same comment here. constraint_expr_to_str() also needs to be fixed.
> 


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

Re: [Non-DoD Source] [PATCH 0/9] Fix issues found by static analyzers

[jwcart2]

On 9/1/19 2:06 PM, Nicolas Iooss wrote:
> Hi,
> 
> In August I ran several free static analyzers in order to find new
> issues in the userspace code. I used:
> 
> * clang's static analyzer (with scripts/run-scan-build)
> * https://lgtm.com/ (I wrote a simple configuration file to analyze the
>    C code, the results are available on
>    https://lgtm.com/projects/g/fishilico/selinux-for-lgtm/ )
> * Facebook's Infer (https://fbinfer.com/, I wrote a script to invoke it
>    like clang's static analyzer)
> 
> Each analyzer gave many results, with several true bugs in them. Here
> are patches that fix some of them. I will post the configuration files
> and scripts I used separately.
> 
> Thanks,
> Nicolas
> 
> Nicolas Iooss (9):
>    semodule-utils: fix comparison with argc
>    libsepol/cil: help static analyzers by aborting when an allocation
>      fails
>    libsepol: do not dereference a failed allocated pointer
>    libsepol: do not dereference scope if it can be NULL
>    libsepol: reset *p to NULL if sepol_module_package_create fails
>    libsepol/cil: do not dereference perm_value_to_cil when it has not
>      been allocated
>    python/chcat: remove unnecessary assignment
>    python/sepolicy: remove unnecessary pass statement
>    libsepol/tests: do not dereference a NULL pointer
> 
>   libsepol/cil/src/cil_binary.c                        | 10 ++++++----
>   libsepol/cil/src/cil_mem.c                           |  5 +++++
>   libsepol/src/avrule_block.c                          |  3 ++-
>   libsepol/src/kernel_to_cil.c                         |  8 +++++---
>   libsepol/src/kernel_to_conf.c                        |  8 +++++---
>   libsepol/src/module.c                                |  4 +++-
>   libsepol/tests/test-common.c                         | 11 +++++++----
>   python/chcat/chcat                                   |  1 -
>   python/sepolicy/sepolicy/__init__.py                 |  1 -
>   semodule-utils/semodule_package/semodule_unpackage.c |  2 +-
>   10 files changed, 34 insertions(+), 19 deletions(-)
> 

I forgot to mention this when I commented on patches 2 and 3, but patches 1 and 
4-9 all look good to me.

I plan on merging these, my take on your patch 2, and your updated patch 3 
tomorrow, unless there are any objections.

Jim


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

Re: [Non-DoD Source] [PATCH 0/9] Fix issues found by static analyzers

[jwcart2]

On 9/16/19 12:46 PM, jwcart2 wrote:
> On 9/1/19 2:06 PM, Nicolas Iooss wrote:
>> Hi,
>>
>> In August I ran several free static analyzers in order to find new
>> issues in the userspace code. I used:
>>
>> * clang's static analyzer (with scripts/run-scan-build)
>> * https://lgtm.com/ (I wrote a simple configuration file to analyze the
>>    C code, the results are available on
>>    https://lgtm.com/projects/g/fishilico/selinux-for-lgtm/ )
>> * Facebook's Infer (https://fbinfer.com/, I wrote a script to invoke it
>>    like clang's static analyzer)
>>
>> Each analyzer gave many results, with several true bugs in them. Here
>> are patches that fix some of them. I will post the configuration files
>> and scripts I used separately.
>>
>> Thanks,
>> Nicolas
>>
>> Nicolas Iooss (9):
>>    semodule-utils: fix comparison with argc
>>    libsepol/cil: help static analyzers by aborting when an allocation
>>      fails
>>    libsepol: do not dereference a failed allocated pointer
>>    libsepol: do not dereference scope if it can be NULL
>>    libsepol: reset *p to NULL if sepol_module_package_create fails
>>    libsepol/cil: do not dereference perm_value_to_cil when it has not
>>      been allocated
>>    python/chcat: remove unnecessary assignment
>>    python/sepolicy: remove unnecessary pass statement
>>    libsepol/tests: do not dereference a NULL pointer
>>
>>   libsepol/cil/src/cil_binary.c                        | 10 ++++++----
>>   libsepol/cil/src/cil_mem.c                           |  5 +++++
>>   libsepol/src/avrule_block.c                          |  3 ++-
>>   libsepol/src/kernel_to_cil.c                         |  8 +++++---
>>   libsepol/src/kernel_to_conf.c                        |  8 +++++---
>>   libsepol/src/module.c                                |  4 +++-
>>   libsepol/tests/test-common.c                         | 11 +++++++----
>>   python/chcat/chcat                                   |  1 -
>>   python/sepolicy/sepolicy/__init__.py                 |  1 -
>>   semodule-utils/semodule_package/semodule_unpackage.c |  2 +-
>>   10 files changed, 34 insertions(+), 19 deletions(-)
>>
> 
> I forgot to mention this when I commented on patches 2 and 3, but patches 1 and 
> 4-9 all look good to me.
> 
> I plan on merging these, my take on your patch 2, and your updated patch 3 
> tomorrow, unless there are any objections.
> 

I have indeed merged your patches 1 and 4-9 along with my take on your patch 2 
and your updated patch 3.

Thanks,
Jim

> Jim
> 
> 


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency