[PATCH] util/qht: fix possible NULL pointer dereference in qht_bucket_remove_entry()

[PATCH] util/qht: fix possible NULL pointer dereference in qht_bucket_remove_entry()

[gerben]

From: Denis Rastyogin <gerben@altlinux.org>

If b->pointers[i] is NULL on the first iteration and
prev has not yet been assigned (i.e., is still NULL),
calling qht_entry_move(orig, pos, prev, QHT_BUCKET_ENTRIES - 1)
can lead to a NULL pointer dereference.

The qht_debug_assert(prev) check does not prevent this issue
because QHT_DEBUG is currently disabled (the #define is commented out).

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Reported-by: Alexey Appolonov <alexey@altlinux.org>
Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
---
 util/qht.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/util/qht.c b/util/qht.c
index 92c6b78759..cb7e367ebb 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -768,6 +768,9 @@ static inline void qht_bucket_remove_entry(struct qht_bucket *orig, int pos)
                 return qht_entry_move(orig, pos, b, i - 1);
             }
             qht_debug_assert(prev);
+            if (!prev) {
+                continue;
+            }
             return qht_entry_move(orig, pos, prev, QHT_BUCKET_ENTRIES - 1);
         }
         prev = b;
-- 
2.42.2

Re: [PATCH] util/qht: fix possible NULL pointer dereference in qht_bucket_remove_entry()

[Alex Bennée]

gerben@altlinux.org writes:

> From: Denis Rastyogin <gerben@altlinux.org>
>
> If b->pointers[i] is NULL on the first iteration and
> prev has not yet been assigned (i.e., is still NULL),
> calling qht_entry_move(orig, pos, prev, QHT_BUCKET_ENTRIES - 1)
> can lead to a NULL pointer dereference.
>
> The qht_debug_assert(prev) check does not prevent this issue
> because QHT_DEBUG is currently disabled (the #define is commented
> out).

The assert is saying you should never see prev as NULL so that would
indicate a bug so we shouldn't be just skipping.

I agree we should probably enable QHT debugging when --enable-debug-tcg
is true.

>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Reported-by: Alexey Appolonov <alexey@altlinux.org>
> Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
> ---
>  util/qht.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/util/qht.c b/util/qht.c
> index 92c6b78759..cb7e367ebb 100644
> --- a/util/qht.c
> +++ b/util/qht.c
> @@ -768,6 +768,9 @@ static inline void qht_bucket_remove_entry(struct qht_bucket *orig, int pos)
>                  return qht_entry_move(orig, pos, b, i - 1);
>              }
>              qht_debug_assert(prev);
> +            if (!prev) {
> +                continue;
> +            }
>              return qht_entry_move(orig, pos, prev, QHT_BUCKET_ENTRIES - 1);
>          }
>          prev = b;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro