On 9/16/19 8:32 AM, Ondrej Mosnacek wrote:
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.
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@xxxxxxxxxxxxx> National Security Agency
