On Sun, Sep 15, 2019 at 9:11 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> 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@xxxxxxx>
> ---
> 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.
