changeset 97:3b468dad363f

value: Fix some issues with regards to yref_t ownership and nested destroying
author Lewin Bormann <lbo@spheniscida.de>
date Mon, 26 Aug 2019 17:01:14 +0200
parents f2b65c8f6a24
children 8498c1faba07
files src/value.c src/value.h src/value_test.c
diffstat 3 files changed, 65 insertions(+), 9 deletions(-) [+]
line wrap: on
line diff
--- a/src/value.c	Mon Aug 26 17:00:47 2019 +0200
+++ b/src/value.c	Mon Aug 26 17:01:14 2019 +0200
@@ -46,7 +46,7 @@
 }
 
 bool yvalue_is_valid(yvalue_id_t id) {
-    return (id & YVALUE_ID_MARKER) == YVALUE_ID_MARKER;
+    return (id & YVALUE_ID_MARKER) == YVALUE_ID_MARKER && id < yvalue_counter;
 }
 
 YREF_TYPE yref_type(yref_t* ref) {
@@ -79,6 +79,19 @@
     return ref;
 }
 
+yref_t yref_clone(yref_t* ref) {
+    switch (yref_type(ref)) {
+        case YREF_SYM:
+            // Copy string.
+            return yref_new_c(ystr_str(&ref->ref.sym));
+        case YREF_ID:
+            return *ref;
+        default:
+            assert(yref_type(ref) == YREF_SYM || yref_type(ref) == YREF_ID);
+            return yref_new();
+    }
+}
+
 yref_t yvalue_clone(yref_t* ref) {
     yvalue_t* orig = yvalue_get(*ref);
     assert(orig->typ == YVALUE_EXPR || orig->typ == YVALUE_FUNC);
@@ -86,7 +99,8 @@
         // Functions are immutable for now.
         return *ref;
     } else if (orig->typ == YVALUE_EXPR) {
-        yvalue_t new = { .typ = YVALUE_EXPR, .value.expr = yexpr_copy(&orig->value.expr) };
+        yvalue_t new = {.typ = YVALUE_EXPR,
+                        .value.expr = yexpr_copy(&orig->value.expr)};
         return yvalue_set(YVALUE_INSERT, &new);
     }
     return yref_new_id(YVALUE_INVALID_ID);
@@ -171,6 +185,8 @@
     if (typ == YREF_ID) return;
     if (typ == YREF_SYM) {
         yvalue_id_t id = yvalue_resolve_or_create_symbol(&ref->ref.sym);
+        // CAREFUL: This may cause double-frees if `ref` is copied from another
+        // ref.
         ystr_destroy(&ref->ref.sym);
         ref->ref.id = id;
     }
@@ -183,13 +199,20 @@
     // ref == YVALUE_INSERT
     if (typ == YREF_ID && ref.ref.id == YVALUE_INSERT.ref.id) {
         // Unnamed value.
-        ref.ref.id = yvalue_counter++;
-        size_t new_index = YVEC_PUSH(&yvalue_table, val);
-        assert(new_index == (yvalue_counter - YVALUE_COUNTER_OFFSET - 1));
+        ref = yref_new();
+        yvalue_t* valp = yvalue_get(ref);
+        assert(valp != NULL);
+        *valp = *val;
         return ref;
     } else {
-        yvalue_resolve_or_create_ref(&ref);
-        yvalue_t* valp = yvalue_get(ref);
+        // Resolve reference.
+        yvalue_id_t id;
+        if (typ == YREF_ID)
+            id = ref.ref.id;
+        else
+            id = yvalue_resolve_or_create_symbol(&ref.ref.sym);
+        assert(yvalue_is_valid(id));
+        yvalue_t* valp = yvalue_get(yref_new_id(id));
         assert(valp != NULL);
         *valp = *val;
         return ref;
--- a/src/value.h	Mon Aug 26 17:00:47 2019 +0200
+++ b/src/value.h	Mon Aug 26 17:01:14 2019 +0200
@@ -46,6 +46,12 @@
 YREF_TYPE yref_type(yref_t* ref);
 
 /**
+ * Create a new reference pointing to the same value as `ref`. The caller
+ * assumes ownership of the returned reference and needs to destroy it later.
+ */
+yref_t yref_clone(yref_t* ref);
+
+/**
  * Create a new unnamed reference.
  */
 yref_t yref_new(void);
@@ -126,6 +132,10 @@
 /**
  * @brief Translate a symbol in `ref` to an ID, or create a new ID. `ref` is
  * updated to be an ID ref. Caller retains ownership of `ref`.
+ *
+ * NOTE: If `ref` is symbolic, this function frees the symbol string. This
+ * means that it is an error to supply a pointer to a reference that is copied
+ * from another reference that is freed later (double-free).
  */
 void yvalue_resolve_or_create_ref(yref_t* ref);
 
@@ -136,7 +146,8 @@
  * deallocated though). If the reference is a symbolic reference that does not
  * exist yet, it is resolved and created first.
  *
- * The reference at which the value was insertes is returned.
+ * The reference at which the value was inserted at is returned (this is only
+ * useful if YVALUE_INSERT is used).
  */
 yref_t yvalue_set(yref_t ref, yvalue_t* val);
 
--- a/src/value_test.c	Mon Aug 26 17:00:47 2019 +0200
+++ b/src/value_test.c	Mon Aug 26 17:01:14 2019 +0200
@@ -20,18 +20,39 @@
 }
 
 void test_ref_create_resolve(void) {
-    ystr_t symbol = ystr_new("rcr_symbol");
+    ystr_t symbol = ystr_new("rcr_symbol_long_long");
     ystr_t symbol2 = ystr_new(NULL);
     ystr_copy(&symbol, &symbol2);
     yref_t ref = yref_new_str(&symbol);  // symbol is now owned by ref.
 
+    yref_t cloned = yref_clone(&ref);
+    assert(cloned.ref.sym.inner.big.base != ref.ref.sym.inner.big.base);
+
     yvalue_resolve_or_create_ref(&ref);
     assert(YVALUE_INVALID_ID != yvalue_resolve_symbol(&symbol2));
     assert(YREF_ID == yref_type(&ref));
+    assert(ref.ref.id == yref_clone(&ref).ref.id);
+    assert(YREF_SYM == yref_type(&cloned));
     assert(ref.ref.id == yvalue_resolve_symbol(&symbol2));
 
     ystr_destroy(&symbol2);
     yref_destroy(&ref);
+    yref_destroy(&cloned);
+}
+
+void test_value_clone(void) {
+    yvalue_t first_value = { .typ = YVALUE_EXPR, .value.expr = yexpr_new() };
+    yexpr_set_str(&first_value.value.expr, ystr_new("a long external test string"));
+    yref_t first = yvalue_set(YVALUE_INSERT, &first_value);
+    yref_t second = yvalue_set(YVALUE_INSERT, &first_value);
+    yref_t cloned = yvalue_clone(&first);
+    assert(first.ref.id != cloned.ref.id);
+
+    *ystr_at(&yvalue_get(first)->value.expr.value.str, 0) = 'A';
+    assert('A' == *ystr_at(&yvalue_get(first)->value.expr.value.str, 0));
+    assert('A' == *ystr_at(&yvalue_get(second)->value.expr.value.str, 0));
+    assert('A' == *ystr_at(&yvalue_get(yref_clone(&first))->value.expr.value.str, 0));
+    assert('a' == *ystr_at(&yvalue_get(cloned)->value.expr.value.str, 0));
 }
 
 void test_value_set(void) {
@@ -83,5 +104,6 @@
 int main(void) {
     test_value_create_resolve();
     test_ref_create_resolve();
+    test_value_clone();
     test_value_set();
 }