* [PATCH v2 1/1] libsepol: do not dereference a failed allocated pointer
@ 2019-09-15 19:10 Nicolas Iooss
2019-09-16 12:32 ` Ondrej Mosnacek
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Iooss @ 2019-09-15 19:10 UTC (permalink / raw)
To: selinux
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 | 16 ++++++++++------
libsepol/src/kernel_to_conf.c | 16 ++++++++++------
2 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index 01f5bc5bba75..ca2e4a9b265b 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;
}
@@ -251,10 +253,12 @@ static char *constraint_expr_to_str(struct policydb *pdb, struct constraint_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 a44ba30a0a44..b49661625e03 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) {
+ while ((new_val = strs_stack_pop(stack)) != NULL) {
+ free(new_val);
+ }
+ strs_stack_destroy(&stack);
}
- strs_stack_destroy(&stack);
return NULL;
}
@@ -247,10 +249,12 @@ static char *constraint_expr_to_str(struct policydb *pdb, struct constraint_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;
}
--
2.22.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] libsepol: do not dereference a failed allocated pointer
2019-09-15 19:10 [PATCH v2 1/1] libsepol: do not dereference a failed allocated pointer Nicolas Iooss
@ 2019-09-16 12:32 ` Ondrej Mosnacek
2019-09-16 14:48 ` [Non-DoD Source] " jwcart2
0 siblings, 1 reply; 3+ messages in thread
From: Ondrej Mosnacek @ 2019-09-16 12:32 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
On Sun, Sep 15, 2019 at 9:11 PM Nicolas Iooss <nicolas.iooss@m4x.org> 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 | 16 ++++++++++------
> libsepol/src/kernel_to_conf.c | 16 ++++++++++------
> 2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index 01f5bc5bba75..ca2e4a9b265b 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;
Why not just replace the 'goto exit;' in the 'rc != 0' branch just
after strs_stack_init() with a plain 'return NULL;'? From my quick
look into the code it seems this would be enough to fix the issue, but
maybe I'm missing something.
> }
> @@ -251,10 +253,12 @@ static char *constraint_expr_to_str(struct policydb *pdb, struct constraint_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 a44ba30a0a44..b49661625e03 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) {
> + while ((new_val = strs_stack_pop(stack)) != NULL) {
> + free(new_val);
> + }
> + strs_stack_destroy(&stack);
> }
> - strs_stack_destroy(&stack);
>
> return NULL;
> }
> @@ -247,10 +249,12 @@ static char *constraint_expr_to_str(struct policydb *pdb, struct constraint_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;
> }
> --
> 2.22.0
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Non-DoD Source] Re: [PATCH v2 1/1] libsepol: do not dereference a failed allocated pointer
2019-09-16 12:32 ` Ondrej Mosnacek
@ 2019-09-16 14:48 ` jwcart2
0 siblings, 0 replies; 3+ messages in thread
From: jwcart2 @ 2019-09-16 14:48 UTC (permalink / raw)
To: Ondrej Mosnacek, Nicolas Iooss; +Cc: SElinux list
On 9/16/19 8:32 AM, Ondrej Mosnacek wrote:
> On Sun, Sep 15, 2019 at 9:11 PM Nicolas Iooss <nicolas.iooss@m4x.org> 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 | 16 ++++++++++------
>> libsepol/src/kernel_to_conf.c | 16 ++++++++++------
>> 2 files changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
>> index 01f5bc5bba75..ca2e4a9b265b 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;
>
> Why not just replace the 'goto exit;' in the 'rc != 0' branch just
> after strs_stack_init() with a plain 'return NULL;'? From my quick
> look into the code it seems this would be enough to fix the issue, but
> maybe I'm missing something.
>
That would work, but I think I like this better. It clearly will prevent a
problem if some future change makes it possible for stack to be NULL in some
other way.
Jim
>> }
>> @@ -251,10 +253,12 @@ static char *constraint_expr_to_str(struct policydb *pdb, struct constraint_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 a44ba30a0a44..b49661625e03 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) {
>> + while ((new_val = strs_stack_pop(stack)) != NULL) {
>> + free(new_val);
>> + }
>> + strs_stack_destroy(&stack);
>> }
>> - strs_stack_destroy(&stack);
>>
>> return NULL;
>> }
>> @@ -247,10 +249,12 @@ static char *constraint_expr_to_str(struct policydb *pdb, struct constraint_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;
>> }
>> --
>> 2.22.0
>>
>
--
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-16 14:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-15 19:10 [PATCH v2 1/1] libsepol: do not dereference a failed allocated pointer Nicolas Iooss
2019-09-16 12:32 ` Ondrej Mosnacek
2019-09-16 14:48 ` [Non-DoD Source] " jwcart2
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).