diff mbox series

[2/2] libsepol: silence potential NULL pointer dereference warning

Message ID 20201003193957.1876526-2-nicolas.iooss@m4x.org
State Accepted
Headers show
Series [1/2] libsepol: drop confusing BUG_ON macro | expand

Commit Message

Nicolas Iooss Oct. 3, 2020, 7:39 p.m. UTC
When find_avtab_node() is called with key->specified & AVTAB_XPERMS and
xperms=NULL, xperms is being dereferenced. This is detected as a
"NULL pointer dereference issue" by static analyzers.

Even though it does not make much sense to call find_avtab_node() in a
way which triggers the NULL pointer dereference issue, static analyzers
have a hard time with calls such as:

    node = find_avtab_node(handle, avtab, &avkey, cond, NULL);

... where xperms=NULL.

So, make the function report an error instead of crashing.

Here is an example of report from clang's static analyzer:
https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/expand.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

James Carter Oct. 15, 2020, 5 p.m. UTC | #1
On Sat, Oct 3, 2020 at 3:41 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> When find_avtab_node() is called with key->specified & AVTAB_XPERMS and
> xperms=NULL, xperms is being dereferenced. This is detected as a
> "NULL pointer dereference issue" by static analyzers.
>
> Even though it does not make much sense to call find_avtab_node() in a
> way which triggers the NULL pointer dereference issue, static analyzers
> have a hard time with calls such as:
>
>     node = find_avtab_node(handle, avtab, &avkey, cond, NULL);
>
> ... where xperms=NULL.
>
> So, make the function report an error instead of crashing.
>
> Here is an example of report from clang's static analyzer:
> https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/src/expand.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 19e48c507236..eac7e4507d02 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1570,17 +1570,22 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
>
>         /* AVTAB_XPERMS entries are not necessarily unique */
>         if (key->specified & AVTAB_XPERMS) {
> -               node = avtab_search_node(avtab, key);
> -               while (node) {
> -                       if ((node->datum.xperms->specified == xperms->specified) &&
> -                               (node->datum.xperms->driver == xperms->driver)) {
> -                               match = 1;
> -                               break;
> +               if (xperms == NULL) {
> +                       ERR(handle, "searching xperms NULL");
> +                       node = NULL;
> +               } else {
> +                       node = avtab_search_node(avtab, key);
> +                       while (node) {
> +                               if ((node->datum.xperms->specified == xperms->specified) &&
> +                                       (node->datum.xperms->driver == xperms->driver)) {
> +                                       match = 1;
> +                                       break;
> +                               }
> +                               node = avtab_search_node_next(node, key->specified);
>                         }
> -                       node = avtab_search_node_next(node, key->specified);
> +                       if (!match)
> +                               node = NULL;
>                 }
> -               if (!match)
> -                       node = NULL;
>         } else {
>                 node = avtab_search_node(avtab, key);
>         }
> --
> 2.28.0
>
Nicolas Iooss Oct. 19, 2020, 9:29 p.m. UTC | #2
On Thu, Oct 15, 2020 at 7:00 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Sat, Oct 3, 2020 at 3:41 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > When find_avtab_node() is called with key->specified & AVTAB_XPERMS and
> > xperms=NULL, xperms is being dereferenced. This is detected as a
> > "NULL pointer dereference issue" by static analyzers.
> >
> > Even though it does not make much sense to call find_avtab_node() in a
> > way which triggers the NULL pointer dereference issue, static analyzers
> > have a hard time with calls such as:
> >
> >     node = find_avtab_node(handle, avtab, &avkey, cond, NULL);
> >
> > ... where xperms=NULL.
> >
> > So, make the function report an error instead of crashing.
> >
> > Here is an example of report from clang's static analyzer:
> > https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>

Thanks. Merged.

Nicolas

> > ---
> >  libsepol/src/expand.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> > index 19e48c507236..eac7e4507d02 100644
> > --- a/libsepol/src/expand.c
> > +++ b/libsepol/src/expand.c
> > @@ -1570,17 +1570,22 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
> >
> >         /* AVTAB_XPERMS entries are not necessarily unique */
> >         if (key->specified & AVTAB_XPERMS) {
> > -               node = avtab_search_node(avtab, key);
> > -               while (node) {
> > -                       if ((node->datum.xperms->specified == xperms->specified) &&
> > -                               (node->datum.xperms->driver == xperms->driver)) {
> > -                               match = 1;
> > -                               break;
> > +               if (xperms == NULL) {
> > +                       ERR(handle, "searching xperms NULL");
> > +                       node = NULL;
> > +               } else {
> > +                       node = avtab_search_node(avtab, key);
> > +                       while (node) {
> > +                               if ((node->datum.xperms->specified == xperms->specified) &&
> > +                                       (node->datum.xperms->driver == xperms->driver)) {
> > +                                       match = 1;
> > +                                       break;
> > +                               }
> > +                               node = avtab_search_node_next(node, key->specified);
> >                         }
> > -                       node = avtab_search_node_next(node, key->specified);
> > +                       if (!match)
> > +                               node = NULL;
> >                 }
> > -               if (!match)
> > -                       node = NULL;
> >         } else {
> >                 node = avtab_search_node(avtab, key);
> >         }
> > --
> > 2.28.0
> >
diff mbox series

Patch

diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 19e48c507236..eac7e4507d02 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1570,17 +1570,22 @@  static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
 
 	/* AVTAB_XPERMS entries are not necessarily unique */
 	if (key->specified & AVTAB_XPERMS) {
-		node = avtab_search_node(avtab, key);
-		while (node) {
-			if ((node->datum.xperms->specified == xperms->specified) &&
-				(node->datum.xperms->driver == xperms->driver)) {
-				match = 1;
-				break;
+		if (xperms == NULL) {
+			ERR(handle, "searching xperms NULL");
+			node = NULL;
+		} else {
+			node = avtab_search_node(avtab, key);
+			while (node) {
+				if ((node->datum.xperms->specified == xperms->specified) &&
+					(node->datum.xperms->driver == xperms->driver)) {
+					match = 1;
+					break;
+				}
+				node = avtab_search_node_next(node, key->specified);
 			}
-			node = avtab_search_node_next(node, key->specified);
+			if (!match)
+				node = NULL;
 		}
-		if (!match)
-			node = NULL;
 	} else {
 		node = avtab_search_node(avtab, key);
 	}