diff mbox series

[OpenWrt-Devel,libubox,9/9] avl: guard against theoretical null pointer dereference

Message ID 20191120115926.23272-10-ynezz@true.cz
State Superseded
Headers show
Series fixes, some unit tests and GitLab CI | expand

Commit Message

Petr Štetiar Nov. 20, 2019, 11:59 a.m. UTC
clang-10 analyzer reports following:

 avl.c:671:25: warning: Access to field 'parent' results in a dereference of a null pointer (loaded from field 'right')
     node->right->parent = parent;
           ~~~~~         ^

Which seems to be impossible to trigger via exported AVL public API, but
it could be probably trigerred by fiddling with the AVL tree node struct
members manually as they are exposed.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 avl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Yousong Zhou Nov. 20, 2019, 12:33 p.m. UTC | #1
On Wed, 20 Nov 2019 at 20:01, Petr Štetiar <ynezz@true.cz> wrote:
>
> clang-10 analyzer reports following:
>
>  avl.c:671:25: warning: Access to field 'parent' results in a dereference of a null pointer (loaded from field 'right')
>      node->right->parent = parent;
>            ~~~~~         ^
>
> Which seems to be impossible to trigger via exported AVL public API, but
> it could be probably trigerred by fiddling with the AVL tree node struct
> members manually as they are exposed.
>

No, theoretically it's still impossible, even if callers fiddled with
these internal members before ;)

The first check (node->left == NULL && node->right == NULL) if
matched, will return.  This commit is on 2nd check (node->left ==
NULL) which implies (node->right != NULL)

                yousong
Petr Štetiar Nov. 20, 2019, 1:33 p.m. UTC | #2
Yousong Zhou <yszhou4tech@gmail.com> [2019-11-20 20:33:06]:

Hi,

thanks for review!

> The first check (node->left == NULL && node->right == NULL) if
> matched, will return.

You can see the code path leading to null pointer dereference for yourself[1].
I wish, that analyzer could output test case directly :-) I wanted to write
test case myself in order to verify it, but it's quite time consuming so I
rather decided to move on with this simple silencer.

1. https://ynezz.gitlab.io/-/openwrt-libubox/-/jobs/355230141/artifacts/build/scan/2019-11-19-163708-203-1/index.html

-- ynezz
Yousong Zhou Nov. 20, 2019, 1:46 p.m. UTC | #3
On Wed, 20 Nov 2019 at 21:33, Petr Štetiar <ynezz@true.cz> wrote:
>
> Yousong Zhou <yszhou4tech@gmail.com> [2019-11-20 20:33:06]:
>
> Hi,
>
> thanks for review!
>
> > The first check (node->left == NULL && node->right == NULL) if
> > matched, will return.
>
> You can see the code path leading to null pointer dereference for yourself[1].
> I wish, that analyzer could output test case directly :-) I wanted to write
> test case myself in order to verify it, but it's quite time consuming so I
> rather decided to move on with this simple silencer.
>
> 1. https://ynezz.gitlab.io/-/openwrt-libubox/-/jobs/355230141/artifacts/build/scan/2019-11-19-163708-203-1/index.html

The graph is very impressive.  It requires the fiddler to first point
node->parent to a stranger whose left and right children are both not
node itself ;)  In that case, I prefer the program just segfault.  No
way it should continue or recover.

                yousong
Yousong Zhou Nov. 20, 2019, 2:01 p.m. UTC | #4
On Wed, 20 Nov 2019 at 21:46, Yousong Zhou <yszhou4tech@gmail.com> wrote:
>
> On Wed, 20 Nov 2019 at 21:33, Petr Štetiar <ynezz@true.cz> wrote:
> >
> > Yousong Zhou <yszhou4tech@gmail.com> [2019-11-20 20:33:06]:
> >
> > Hi,
> >
> > thanks for review!
> >
> > > The first check (node->left == NULL && node->right == NULL) if
> > > matched, will return.
> >
> > You can see the code path leading to null pointer dereference for yourself[1].
> > I wish, that analyzer could output test case directly :-) I wanted to write
> > test case myself in order to verify it, but it's quite time consuming so I
> > rather decided to move on with this simple silencer.
> >
> > 1. https://ynezz.gitlab.io/-/openwrt-libubox/-/jobs/355230141/artifacts/build/scan/2019-11-19-163708-203-1/index.html
>
> The graph is very impressive.  It requires the fiddler to first point
> node->parent to a stranger whose left and right children are both not
> node itself ;)  In that case, I prefer the program just segfault.  No
> way it should continue or recover.
>

By the way, will assert(node-parent != NULL) suffice to inform the
analyzer the underlying details?  If it does, we could also apply it
to b64_encode(), b64_decode().

                yousong
Petr Štetiar Nov. 20, 2019, 9:29 p.m. UTC | #5
Yousong Zhou <yszhou4tech@gmail.com> [2019-11-20 22:01:22]:

> By the way, will assert(node-parent != NULL) suffice to inform the
> analyzer the underlying details?  If it does, we could also apply it
> to b64_encode(), b64_decode().

BTW in libnl-tiny I've learned, that using assert() in CMake release builds
needs fiddling with C flags as CMake adds -DNDEBUG to release build C flags
which in turn disable assert().

Anyway, I agree, that crashing with invalid inputs is the proper way, so I've
reworked it with assert() as it works for analyzer as well.

-- ynezz
diff mbox series

Patch

diff --git a/avl.c b/avl.c
index 8d0bf65aaa5b..e5584051ab94 100644
--- a/avl.c
+++ b/avl.c
@@ -668,7 +668,8 @@  avl_delete_worker(struct avl_tree *tree, struct avl_node *node)
       return;
     }
 
-    node->right->parent = parent;
+    if (node->right)
+      node->right->parent = parent;
 
     if (parent->left == node)
       parent->left = node->right;