| Message ID | 20220629072055.2653695-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d2fecbb97b79 |
| Headers | show |
| Series | [userspace,1/1] libsepol: initialize s in constraint_expr_eval_reason | expand |
On Wed, Jun 29, 2022 at 3:37 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > clang's static analyzer reports that s[0] can be uninitialized when used > in: > > sprintf(tmp_buf, "%s %s\n", > xcontext ? "Validatetrans" : "Constraint", > s[0] ? "GRANTED" : "DENIED"); > > Silence this false-positive issue by making s always initialized. > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> Acked-by: James Carter <jwcart2@gmail.com> > --- > libsepol/src/services.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libsepol/src/services.c b/libsepol/src/services.c > index d7510e9dae51..db769cdcfaf9 100644 > --- a/libsepol/src/services.c > +++ b/libsepol/src/services.c > @@ -394,7 +394,7 @@ static int constraint_expr_eval_reason(context_struct_t *scontext, > role_datum_t *r1, *r2; > mls_level_t *l1, *l2; > constraint_expr_t *e; > - int s[CEXPR_MAXDEPTH]; > + int s[CEXPR_MAXDEPTH] = {}; > int sp = -1; > char tmp_buf[128]; > > -- > 2.36.1 >
On Wed, 29 Jun 2022 at 22:07, James Carter <jwcart2@gmail.com> wrote: > > On Wed, Jun 29, 2022 at 3:37 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > > > clang's static analyzer reports that s[0] can be uninitialized when used > > in: > > > > sprintf(tmp_buf, "%s %s\n", > > xcontext ? "Validatetrans" : "Constraint", > > s[0] ? "GRANTED" : "DENIED"); > > The trace for the reports shows: 441 for (e = constraint->expr; e; e = e->next) { Loop condition is false. Execution continues on line 708 and clang-tidy also reports: libsepol/src/services.c:715:16: warning: Call to 'calloc' has an allocation size of 0 bytes [clang-analyzer-optin.portability.UnixAPI] answer_list = calloc(expr_count, sizeof(*answer_list)); ^ ~~~~~~~~~~ libsepol/src/services.c:433:6: note: Assuming 'class_buf' is non-null if (!class_buf) { ^~~~~~~~~~ libsepol/src/services.c:433:2: note: Taking false branch if (!class_buf) { ^ libsepol/src/services.c:439:2: note: The value 0 is assigned to 'expr_counter' expr_counter = 0; ^~~~~~~~~~~~~~~~ libsepol/src/services.c:441:2: note: Loop condition is false. Execution continues on line 708 for (e = constraint->expr; e; e = e->next) { ^ libsepol/src/services.c:708:2: note: The value 0 is assigned to 'expr_count' expr_count = expr_counter; ^~~~~~~~~~~~~~~~~~~~~~~~~ libsepol/src/services.c:715:16: note: Call to 'calloc' has an allocation size of 0 bytes answer_list = calloc(expr_count, sizeof(*answer_list)); ^ ~~~~~~~~~~ I think the root cause is the possibility of `constraint->expr` being NULL, i.e. the constraint having no expression, which seems invalid to me. Maybe add a check `if(!constraint->expr) { BUG(); return -EINVAL; }`? Also validation should probably catch this at https://github.com/SELinuxProject/selinux/blob/956bda08f6183078f13b70f6aa27d0529a3ec20a/libsepol/src/policydb_validate.c#L238 + if (!cons->expr) + goto bad; + for (cexp = cons->expr; cexp; cexp = cexp->next) { > > Silence this false-positive issue by making s always initialized. > > > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > > Acked-by: James Carter <jwcart2@gmail.com> > > > --- > > libsepol/src/services.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libsepol/src/services.c b/libsepol/src/services.c > > index d7510e9dae51..db769cdcfaf9 100644 > > --- a/libsepol/src/services.c > > +++ b/libsepol/src/services.c > > @@ -394,7 +394,7 @@ static int constraint_expr_eval_reason(context_struct_t *scontext, > > role_datum_t *r1, *r2; > > mls_level_t *l1, *l2; > > constraint_expr_t *e; > > - int s[CEXPR_MAXDEPTH]; > > + int s[CEXPR_MAXDEPTH] = {}; > > int sp = -1; > > char tmp_buf[128]; > > > > -- > > 2.36.1 > >
On Wed, Jun 29, 2022 at 4:06 PM James Carter <jwcart2@gmail.com> wrote: > > On Wed, Jun 29, 2022 at 3:37 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > > > clang's static analyzer reports that s[0] can be uninitialized when used > > in: > > > > sprintf(tmp_buf, "%s %s\n", > > xcontext ? "Validatetrans" : "Constraint", > > s[0] ? "GRANTED" : "DENIED"); > > > > Silence this false-positive issue by making s always initialized. > > > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > > Acked-by: James Carter <jwcart2@gmail.com> > Merged. Thanks, Jim > > --- > > libsepol/src/services.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libsepol/src/services.c b/libsepol/src/services.c > > index d7510e9dae51..db769cdcfaf9 100644 > > --- a/libsepol/src/services.c > > +++ b/libsepol/src/services.c > > @@ -394,7 +394,7 @@ static int constraint_expr_eval_reason(context_struct_t *scontext, > > role_datum_t *r1, *r2; > > mls_level_t *l1, *l2; > > constraint_expr_t *e; > > - int s[CEXPR_MAXDEPTH]; > > + int s[CEXPR_MAXDEPTH] = {}; > > int sp = -1; > > char tmp_buf[128]; > > > > -- > > 2.36.1 > >
diff --git a/libsepol/src/services.c b/libsepol/src/services.c index d7510e9dae51..db769cdcfaf9 100644 --- a/libsepol/src/services.c +++ b/libsepol/src/services.c @@ -394,7 +394,7 @@ static int constraint_expr_eval_reason(context_struct_t *scontext, role_datum_t *r1, *r2; mls_level_t *l1, *l2; constraint_expr_t *e; - int s[CEXPR_MAXDEPTH]; + int s[CEXPR_MAXDEPTH] = {}; int sp = -1; char tmp_buf[128];
clang's static analyzer reports that s[0] can be uninitialized when used in: sprintf(tmp_buf, "%s %s\n", xcontext ? "Validatetrans" : "Constraint", s[0] ? "GRANTED" : "DENIED"); Silence this false-positive issue by making s always initialized. Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libsepol/src/services.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)