diff mbox series

[ovs-dev,v4,07/15] hmap: implement UB-safe hmap pop iterator

Message ID 20220309161023.3347758-8-amorenoz@redhat.com
State Superseded
Headers show
Series Fix undefined behavior in loop macros | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/intel-ovs-compilation success test: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Adrián Moreno March 9, 2022, 4:10 p.m. UTC 
HMAP_FOR_EACH_POP iterator has an additional difficulty, which is the
use of two iterator variables of different types.

In order to re-write this loop in a UB-safe manner, create a iterator
struct to be used as loop variable.

Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/openvswitch/hmap.h | 31 +++++++++++++++++++------------
 tests/test-hmap.c          |  1 +
 2 files changed, 20 insertions(+), 12 deletions(-)

Comments

0-day Robot March 9, 2022, 5:31 p.m. UTC | #1 
Bleep bloop.  Greetings Adrian Moreno, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Use ovs_assert() in place of assert()
#77 FILE: tests/test-hmap.c:320:
        assert(e == NULL);

Lines checked: 84, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Eelco Chaudron March 21, 2022, 10:32 a.m. UTC | #2 
On 9 Mar 2022, at 17:10, Adrian Moreno wrote:

> HMAP_FOR_EACH_POP iterator has an additional difficulty, which is the
> use of two iterator variables of different types.
>
> In order to re-write this loop in a UB-safe manner, create a iterator
> struct to be used as loop variable.
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>


Changes look good to me!

Acked-by: Eelco Chaudron <echaudro@redhat.com>
diff mbox series

Patch

diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h
index 610ad9987..68c284cf1 100644
--- a/include/openvswitch/hmap.h
+++ b/include/openvswitch/hmap.h
@@ -199,26 +199,33 @@  bool hmap_contains(const struct hmap *, const struct hmap_node *);
          CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);            \
          UPDATE_MULTIVAR(NODE, hmap_next(HMAP, ITER_VAR(NODE))))
 
-static inline struct hmap_node *
-hmap_pop_helper__(struct hmap *hmap, size_t *bucket) {
+struct hmap_pop_helper_iter__ {
+    size_t bucket;
+    struct hmap_node *node;
+};
+
+static inline void
+hmap_pop_helper__(struct hmap *hmap, struct hmap_pop_helper_iter__ *iter) {
 
-    for (; *bucket <= hmap->mask; (*bucket)++) {
-        struct hmap_node *node = hmap->buckets[*bucket];
+    for (; iter->bucket <= hmap->mask; (iter->bucket)++) {
+        struct hmap_node *node = hmap->buckets[iter->bucket];
 
         if (node) {
             hmap_remove(hmap, node);
-            return node;
+            iter->node = node;
+            return;
         }
     }
-
-    return NULL;
+    iter->node = NULL;
 }
 
-#define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP)                               \
-    for (size_t bucket__ = 0;                                               \
-         INIT_CONTAINER(NODE, hmap_pop_helper__(HMAP, &bucket__), MEMBER),  \
-         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))                    \
-         || ((NODE = NULL), false);)
+#define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP)                                 \
+    for (struct hmap_pop_helper_iter__ ITER_VAR(NODE) = { 0, NULL };          \
+         hmap_pop_helper__(HMAP, &ITER_VAR(NODE)),                            \
+         (ITER_VAR(NODE).node != NULL) ?                                      \
+            (((NODE) = OBJECT_CONTAINING(ITER_VAR(NODE).node,                 \
+                                         NODE, MEMBER)),1):                   \
+            (((NODE) = NULL), 0);)
 
 static inline struct hmap_node *hmap_first(const struct hmap *);
 static inline struct hmap_node *hmap_next(const struct hmap *,
diff --git a/tests/test-hmap.c b/tests/test-hmap.c
index a40ac8953..47b475538 100644
--- a/tests/test-hmap.c
+++ b/tests/test-hmap.c
@@ -317,6 +317,7 @@  test_hmap_for_each_pop(hash_func *hash)
             i++;
         }
         assert(i == n);
+        assert(e == NULL);
 
         hmap_destroy(&hmap);
     }