| Message ID | 20191120115926.23272-10-ynezz@true.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fixes, some unit tests and GitLab CI | expand |
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
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
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
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
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 --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;
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(-)