From 40320aaa4a2016d0c5629604d09a1d018dd55dab Mon Sep 17 00:00:00 2001
From: Laria Carolin Chabowski <laria@laria.me>
Date: Tue, 15 Feb 2022 22:32:15 +0100
Subject: [PATCH] Fix copying dicts

We didn't properly copy the opaque value before (a heap-allocated allocator
pointer), resulting in a use-after-free of the opaque on the copied map if
the old map was destroyed.
---
 src/hashmap.c | 29 +++++++++++++++++++++++------
 src/hashmap.h |  5 ++++-
 src/value.c   | 18 ++++++++++++++++--
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/src/hashmap.c b/src/hashmap.c
index 9bf1847..a34908d 100644
--- a/src/hashmap.c
+++ b/src/hashmap.c
@@ -94,11 +94,22 @@ copy_value(const struct apfl_hashmap map, void *dest, void *src)
     }
 }
 
-static void
-on_deinit(const struct apfl_hashmap map)
+static bool
+copy_opaque(const struct apfl_hashmap map, void **dest, void *src)
 {
-    if (HAS_CALLBACK(map, on_deinit)) {
-        INVOKE_CALLBACK_NOARGS(map, on_deinit);
+    if (HAS_CALLBACK(map, copy_opaque)) {
+        return map.callbacks.copy_opaque(dest, src);
+    } else {
+        *dest = src;
+        return true;
+    }
+}
+
+static void
+deinit_opaque(const struct apfl_hashmap map)
+{
+    if (HAS_CALLBACK(map, deinit_opaque)) {
+        INVOKE_CALLBACK_NOARGS(map, deinit_opaque);
     }
 }
 
@@ -363,7 +374,7 @@ apfl_hashmap_deinit(struct apfl_hashmap *map)
         map->buckets = NULL;
     }
 
-    on_deinit(*map);
+    deinit_opaque(*map);
 }
 
 struct apfl_hashmap
@@ -381,7 +392,13 @@ apfl_hashmap_copy(struct apfl_hashmap *dst, struct apfl_hashmap src)
     size_t keysize = src.keysize;
     size_t valsize = src.valsize;
 
-    if (!hashmap_init(dst, src.allocator, src.callbacks, src.nbuckets, keysize, valsize)) {
+    struct apfl_hashmap_callbacks dst_callbacks = src.callbacks;
+    if (!copy_opaque(src, &dst_callbacks.opaque, src.callbacks.opaque)) {
+        return false;
+    }
+
+    if (!hashmap_init(dst, src.allocator, dst_callbacks, src.nbuckets, keysize, valsize)) {
+        deinit_opaque(*dst);
         return false;
     }
 
diff --git a/src/hashmap.h b/src/hashmap.h
index 68ca765..444542e 100644
--- a/src/hashmap.h
+++ b/src/hashmap.h
@@ -45,8 +45,11 @@ struct apfl_hashmap_callbacks {
     // If not provided, the bytes will be copied with memcpy.
     void      (*copy_value)   (void *opaque, void *dest, void *src);
 
+    // Copies the opaque value. Returns true on success, false on failure.
+    bool      (*copy_opaque)  (void **dst, void *src);
+
     // Called on deinitialization of the hashmap, if provided.
-    void      (*on_deinit)    (void *opaque);
+    void      (*deinit_opaque)(void *opaque);
 };
 
 struct apfl_hashmap {
diff --git a/src/value.c b/src/value.c
index 8d95f92..04576fb 100644
--- a/src/value.c
+++ b/src/value.c
@@ -85,8 +85,21 @@ dict_copy_key_or_value(void *opaque, void *_dest, void *_src)
     *dest = apfl_value_incref(*src);
 }
 
+static bool
+dict_copy_opaque(void **_dst, void *src)
+{
+    struct apfl_allocator *allocator = src;
+    struct apfl_allocator *dst;
+    if ((dst = ALLOC_OBJ(*allocator, struct apfl_allocator)) == NULL) {
+        return false;
+    }
+    *dst = *allocator;
+    *_dst = dst;
+    return true;
+}
+
 static void
-dict_on_deinit(void *opaque)
+dict_deinit_opaque(void *opaque)
 {
     struct apfl_allocator *allocator = opaque;
     FREE_OBJ(*allocator, allocator);
@@ -112,7 +125,8 @@ init_hashmap_for_dict(struct apfl_allocator allocator, struct apfl_hashmap *map)
             .destroy_value = dict_destroy_key_or_value,
             .copy_key      = dict_copy_key_or_value,
             .copy_value    = dict_copy_key_or_value,
-            .on_deinit     = dict_on_deinit,
+            .copy_opaque   = dict_copy_opaque,
+            .deinit_opaque = dict_deinit_opaque,
         },
         sizeof(struct apfl_value),
         sizeof(struct apfl_value)