| Message ID | 20190915191039.107622-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/1] libsepol: do not dereference a failed allocated pointer | expand |
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 >
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 >> >
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; }
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(-)