| Message ID | 20180305225820.23610-2-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 03/05/2018 05:58 PM, Nicolas Iooss wrote: > cil_tree_print_expr() calls cil_expr_to_string() in order to compute a > string expression into expr_str. If this function fails, expr_str is > left unitialized but its value is dereferenced with: > > cil_log(CIL_INFO, "%s)", expr_str); > > Prevent such an issue by checking cil_expr_to_string()'s return value > before using expr_str. > > This issue has been found with clang's static analyzer. > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > --- > libsepol/cil/src/cil_tree.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c > index d36401b41dba..b394a9d849df 100644 > --- a/libsepol/cil/src/cil_tree.c > +++ b/libsepol/cil/src/cil_tree.c > @@ -503,15 +503,19 @@ exit: > void cil_tree_print_expr(struct cil_list *datum_expr, struct cil_list *str_expr) > { > char *expr_str; > + int rc; > > cil_log(CIL_INFO, "("); > > if (datum_expr != NULL) { > - cil_expr_to_string(datum_expr, &expr_str); > + rc = cil_expr_to_string(datum_expr, &expr_str); > } else { > - cil_expr_to_string(str_expr, &expr_str); > + rc = cil_expr_to_string(str_expr, &expr_str); > + } > + if (rc < 0) { > + cil_log(CIL_INFO, "ERROR)"); > + return; Wondering if we should abort or return an error to the caller instead of just logging an error and returning? > } > - > cil_log(CIL_INFO, "%s)", expr_str); > free(expr_str); > } >
On 03/05/2018 05:58 PM, Nicolas Iooss wrote: > cil_tree_print_expr() calls cil_expr_to_string() in order to compute a > string expression into expr_str. If this function fails, expr_str is > left unitialized but its value is dereferenced with: > > cil_log(CIL_INFO, "%s)", expr_str); > > Prevent such an issue by checking cil_expr_to_string()'s return value > before using expr_str. > > This issue has been found with clang's static analyzer. > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> Acked-by: James Carter <jwcart2@tycho.nsa.gov> > --- > libsepol/cil/src/cil_tree.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c > index d36401b41dba..b394a9d849df 100644 > --- a/libsepol/cil/src/cil_tree.c > +++ b/libsepol/cil/src/cil_tree.c > @@ -503,15 +503,19 @@ exit: > void cil_tree_print_expr(struct cil_list *datum_expr, struct cil_list *str_expr) > { > char *expr_str; > + int rc; > > cil_log(CIL_INFO, "("); > > if (datum_expr != NULL) { > - cil_expr_to_string(datum_expr, &expr_str); > + rc = cil_expr_to_string(datum_expr, &expr_str); > } else { > - cil_expr_to_string(str_expr, &expr_str); > + rc = cil_expr_to_string(str_expr, &expr_str); > + } > + if (rc < 0) { > + cil_log(CIL_INFO, "ERROR)"); > + return; > } > - > cil_log(CIL_INFO, "%s)", expr_str); > free(expr_str); > } >
On Tue, Mar 6, 2018 at 10:29 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 03/05/2018 05:58 PM, Nicolas Iooss wrote: >> cil_tree_print_expr() calls cil_expr_to_string() in order to compute a >> string expression into expr_str. If this function fails, expr_str is >> left unitialized but its value is dereferenced with: >> >> cil_log(CIL_INFO, "%s)", expr_str); >> >> Prevent such an issue by checking cil_expr_to_string()'s return value >> before using expr_str. >> >> This issue has been found with clang's static analyzer. >> >> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> >> --- >> libsepol/cil/src/cil_tree.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c >> index d36401b41dba..b394a9d849df 100644 >> --- a/libsepol/cil/src/cil_tree.c >> +++ b/libsepol/cil/src/cil_tree.c >> @@ -503,15 +503,19 @@ exit: >> void cil_tree_print_expr(struct cil_list *datum_expr, struct cil_list *str_expr) >> { >> char *expr_str; >> + int rc; >> >> cil_log(CIL_INFO, "("); >> >> if (datum_expr != NULL) { >> - cil_expr_to_string(datum_expr, &expr_str); >> + rc = cil_expr_to_string(datum_expr, &expr_str); >> } else { >> - cil_expr_to_string(str_expr, &expr_str); >> + rc = cil_expr_to_string(str_expr, &expr_str); >> + } >> + if (rc < 0) { >> + cil_log(CIL_INFO, "ERROR)"); >> + return; > > Wondering if we should abort or return an error to the caller instead of just logging an error and returning? I believe this depends on the usage of cil_tree_print_expr(). I have not found any caller of cil_tree_print_* outside of libsepol/cil/src/cil_tree.c so I guess this interface is only used by developers for debugging purpose when playing with libsepol's CIL objects (being able to quickly print an object in a human-readable form is a very valuable feature). Am I right? The current interface of ignoring all errors and logging as much as possible without aborting seems consistent with such a use-case. If there is an interest in propagating errors found by cil_tree_print_expr() back to its callers, I can spend some time to clean up the code. Otherwise I will send other patches (once they are in a suitable state) to fix some issues that clang's static analyzer find. Thanks, Nicolas
On 03/08/2018 03:42 PM, Nicolas Iooss wrote: > On Tue, Mar 6, 2018 at 10:29 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On 03/05/2018 05:58 PM, Nicolas Iooss wrote: >>> cil_tree_print_expr() calls cil_expr_to_string() in order to compute a >>> string expression into expr_str. If this function fails, expr_str is >>> left unitialized but its value is dereferenced with: >>> >>> cil_log(CIL_INFO, "%s)", expr_str); >>> >>> Prevent such an issue by checking cil_expr_to_string()'s return value >>> before using expr_str. >>> >>> This issue has been found with clang's static analyzer. >>> >>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> >>> --- >>> libsepol/cil/src/cil_tree.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c >>> index d36401b41dba..b394a9d849df 100644 >>> --- a/libsepol/cil/src/cil_tree.c >>> +++ b/libsepol/cil/src/cil_tree.c >>> @@ -503,15 +503,19 @@ exit: >>> void cil_tree_print_expr(struct cil_list *datum_expr, struct cil_list *str_expr) >>> { >>> char *expr_str; >>> + int rc; >>> >>> cil_log(CIL_INFO, "("); >>> >>> if (datum_expr != NULL) { >>> - cil_expr_to_string(datum_expr, &expr_str); >>> + rc = cil_expr_to_string(datum_expr, &expr_str); >>> } else { >>> - cil_expr_to_string(str_expr, &expr_str); >>> + rc = cil_expr_to_string(str_expr, &expr_str); >>> + } >>> + if (rc < 0) { >>> + cil_log(CIL_INFO, "ERROR)"); >>> + return; >> >> Wondering if we should abort or return an error to the caller instead of just logging an error and returning? > > I believe this depends on the usage of cil_tree_print_expr(). I have > not found any caller of cil_tree_print_* outside of > libsepol/cil/src/cil_tree.c so I guess this interface is only used by > developers for debugging purpose when playing with libsepol's CIL > objects (being able to quickly print an object in a human-readable > form is a very valuable feature). Am I right? The current interface of > ignoring all errors and logging as much as possible without aborting > seems consistent with such a use-case. > > If there is an interest in propagating errors found by > cil_tree_print_expr() back to its callers, I can spend some time to > clean up the code. Otherwise I will send other patches (once they are > in a suitable state) to fix some issues that clang's static analyzer > find. You are correct, and James agreed with your approach, which is why I merged the patch as is.
diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c index d36401b41dba..b394a9d849df 100644 --- a/libsepol/cil/src/cil_tree.c +++ b/libsepol/cil/src/cil_tree.c @@ -503,15 +503,19 @@ exit: void cil_tree_print_expr(struct cil_list *datum_expr, struct cil_list *str_expr) { char *expr_str; + int rc; cil_log(CIL_INFO, "("); if (datum_expr != NULL) { - cil_expr_to_string(datum_expr, &expr_str); + rc = cil_expr_to_string(datum_expr, &expr_str); } else { - cil_expr_to_string(str_expr, &expr_str); + rc = cil_expr_to_string(str_expr, &expr_str); + } + if (rc < 0) { + cil_log(CIL_INFO, "ERROR)"); + return; } - cil_log(CIL_INFO, "%s)", expr_str); free(expr_str); }
cil_tree_print_expr() calls cil_expr_to_string() in order to compute a string expression into expr_str. If this function fails, expr_str is left unitialized but its value is dereferenced with: cil_log(CIL_INFO, "%s)", expr_str); Prevent such an issue by checking cil_expr_to_string()'s return value before using expr_str. This issue has been found with clang's static analyzer. Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libsepol/cil/src/cil_tree.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)