Bladeren bron

fix(containers): complete element ownership in Series/Fifo/Lifo/PriorityQueue — insertion already dups via t_dup_func and clear() destroys, but pop_start, remove_first_where/remove_all_where, Fifo/Lifo/PriorityQueue pop and iteration, and the queue destructors never released the stored value (one leaked ref per removal, whole contents leaked at finalize); remove_items additionally advanced through the deleted node (node->next after delete — use-after-free on every remove_all_where past a match, previously surviving by allocator luck; a regression test made it a deterministic segfault). All removal paths now null the item's value before delete so valac emits the t_destroy_func release, and remove_items walks a saved next pointer. No signature, vapi, or ABI change: add(T) keeps copy-on-insert semantics, pop/remove transfer their dup out as before, so the only observable difference is the container's own reference is dropped. Regression tests pin the contract: statement temporaries survive add+clear cycles, pop/remove/clear/destructor restore refcounts, and 603-test suite passes with zero valgrind errors on a probe that adds statement-temporary strings and objects and reads them after (the daemon-side saddle pin.c workaround is unnecessary against this build)

clanker 1 maand geleden
bovenliggende
commit
b2b135a6fd

+ 3 - 0
src/lib/DataStructures/Fifo.vala

@@ -80,6 +80,7 @@ namespace Invercargill.DataStructures {
                 throw new SequenceError.NO_ELEMENTS("No elements in unblocked queue to pop");
             }
             var result = item->item;
+            item->item = null;
             delete item;
             return result;
         }
@@ -125,6 +126,7 @@ namespace Invercargill.DataStructures {
             first_item = item->next_item;            
             mutex.unlock();
             var result = item->item;
+            item->item = null;
             delete item;
             return result;
         }
@@ -144,6 +146,7 @@ namespace Invercargill.DataStructures {
             while(n != null) {
                 var c = n;
                 n = n->next_item;
+                c->item = null;
                 delete c;
             }
         }

+ 3 - 0
src/lib/DataStructures/Lifo.vala

@@ -79,6 +79,7 @@ namespace Invercargill.DataStructures {
                 throw new SequenceError.NO_ELEMENTS("No elements in unblocked stack to pop");
             }
             var result = item->item;
+            item->item = null;
             delete item;
             return result;
         }
@@ -113,6 +114,7 @@ namespace Invercargill.DataStructures {
             top_item = item->next_item;            
             mutex.unlock();
             var result = item->item;
+            item->item = null;
             delete item;
             return result;
         }
@@ -132,6 +134,7 @@ namespace Invercargill.DataStructures {
             while(n != null) {
                 var c = n;
                 n = n->next_item;
+                c->item = null;
                 delete c;
             }
         }

+ 5 - 1
src/lib/DataStructures/PriorityQueue.vala

@@ -73,6 +73,7 @@ namespace Invercargill.DataStructures {
                 bubble_down(0);
             }
             
+            item->item = null;
             delete item;
             mutex.unlock();
             return result;
@@ -163,6 +164,7 @@ namespace Invercargill.DataStructures {
                 bubble_down(0);
             }
             
+            item->item = null;
             delete item;
             mutex.unlock();
             return result;
@@ -179,7 +181,9 @@ namespace Invercargill.DataStructures {
 
         ~PriorityQueue() {
             for (uint i = 0; i < items.length; i++) {
-                delete items.get(i);
+                var queue_item = items.get(i);
+                queue_item->item = null;
+                delete queue_item;
             }
         }
 

+ 14 - 9
src/lib/DataStructures/Series.vala

@@ -129,6 +129,7 @@ namespace Invercargill.DataStructures {
                     var old_root = root;
                     root = root->next;
                     item = old_root->value;
+                    old_root->value = null;
                     delete old_root;
                     n_items--;
                     if(root == null) {
@@ -154,18 +155,22 @@ namespace Invercargill.DataStructures {
                 SeriesItem<T>* previous = null;
                 SeriesItem<T>* node = root;
                 while(node != null) {
-                    if(predicate(node->value)) {
-                        if(previous == null) {
-                            root = node->next;
-                        }
-                        else {
-                            previous->next = node->next;
-                        }
-                        delete node;
-                        n_items--;
+                        if(predicate(node->value)) {
+                            SeriesItem<T>* next = node->next;
+                            if(previous == null) {
+                                root = next;
+                            }
+                            else {
+                                previous->next = next;
+                            }
+                            node->value = null;
+                            delete node;
+                            n_items--;
                         if(first_only) {
                             break;
                         }
+                        node = next;
+                        continue;
                     }
                     previous = node;
                     node = node->next;

+ 44 - 0
src/tests/Integration/Fifo.vala

@@ -209,4 +209,48 @@ void fifo_tests() {
         assert_cmpstr("second", CompareOperator.EQ, fifo.pop());
     });
 
+    Test.add_func("/invercargill/fifo/pop_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var fifo = new Fifo<OwnershipProbe>();
+        fifo.push(probe);
+        fifo.push(probe);
+        assert(probe.ref_count == 3);
+        fifo.unblock();
+        while(true) {
+            try {
+                fifo.pop();
+            }
+            catch(SequenceError e) {
+                break;
+            }
+        }
+        assert(probe.ref_count == 1);
+    });
+
+    Test.add_func("/invercargill/fifo/destructor_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var fifo = new Fifo<OwnershipProbe>();
+        fifo.push(probe);
+        assert(probe.ref_count == 2);
+        fifo = null;
+        assert(probe.ref_count == 1);
+    });
+
+    Test.add_func("/invercargill/fifo/stores_owned_copies_of_temporaries", () => {
+        var fifo = new Fifo<string>();
+        string source = "stable";
+        for(int i = 0; i < 50; i++) {
+            fifo.push("temporary-%d".printf(i));
+            fifo.push(source.dup());
+        }
+        fifo.unblock();
+        uint found = 0;
+        foreach(var item in fifo) {
+            if(item.has_prefix("temporary-") || item == "stable") {
+                found++;
+            }
+        }
+        assert(found == 100);
+    });
+
 }

+ 27 - 0
src/tests/Integration/Lifo.vala

@@ -209,4 +209,31 @@ void lifo_tests() {
         assert_cmpstr("first", CompareOperator.EQ, lifo.pop());
     });
 
+    Test.add_func("/invercargill/lifo/pop_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var lifo = new Lifo<OwnershipProbe>();
+        lifo.push(probe);
+        lifo.push(probe);
+        assert(probe.ref_count == 3);
+        lifo.unblock();
+        while(true) {
+            try {
+                lifo.pop();
+            }
+            catch(SequenceError e) {
+                break;
+            }
+        }
+        assert(probe.ref_count == 1);
+    });
+
+    Test.add_func("/invercargill/lifo/destructor_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var lifo = new Lifo<OwnershipProbe>();
+        lifo.push(probe);
+        assert(probe.ref_count == 2);
+        lifo = null;
+        assert(probe.ref_count == 1);
+    });
+
 }

+ 27 - 0
src/tests/Integration/PriorityQueue.vala

@@ -272,4 +272,31 @@ void priority_queue_tests() {
         assert_cmpstr("zebra", CompareOperator.EQ, pq.pop());
     });
 
+    Test.add_func("/invercargill/priority_queue/pop_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var pq = new PriorityQueue<OwnershipProbe>((a, b) => 0);
+        pq.push(probe);
+        pq.push(probe);
+        assert(probe.ref_count == 3);
+        pq.unblock();
+        while(true) {
+            try {
+                pq.pop();
+            }
+            catch(SequenceError e) {
+                break;
+            }
+        }
+        assert(probe.ref_count == 1);
+    });
+
+    Test.add_func("/invercargill/priority_queue/destructor_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var pq = new PriorityQueue<OwnershipProbe>((a, b) => 0);
+        pq.push(probe);
+        assert(probe.ref_count == 2);
+        pq = null;
+        assert(probe.ref_count == 1);
+    });
+
 }

+ 61 - 0
src/tests/Integration/Series.vala

@@ -32,4 +32,65 @@ void series_tests() {
             assert(array[i] == items[i]);
         }
     });
+
+    Test.add_func("/invercargill/structure/series/stores_owned_copies_of_temporaries", () => {
+        var series = new Series<string>();
+        string source = "stable";
+        for(int i = 0; i < 100; i++) {
+            series.add("temporary-%d".printf(i));
+            series.add(source.dup());
+        }
+        series.clear();
+        for(int i = 0; i < 100; i++) {
+            series.add("temporary-%d".printf(i));
+            series.add(source.dup());
+        }
+        uint found = 0;
+        foreach(var item in series) {
+            if(item.has_prefix("temporary-") || item == "stable") {
+                found++;
+            }
+        }
+        assert(found == series.count());
+        assert(found == 200);
+    });
+
+    Test.add_func("/invercargill/structure/series/pop_start_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var series = new Series<OwnershipProbe>();
+        series.add(probe);
+        assert(probe.ref_count == 2);
+        try {
+            series.pop_start();
+        }
+        catch(SequenceError e) {
+            assert_not_reached();
+        }
+        assert(probe.ref_count == 1);
+    });
+
+    Test.add_func("/invercargill/structure/series/remove_where_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var kept = new OwnershipProbe();
+        var series = new Series<OwnershipProbe>();
+        series.add(probe);
+        series.add(kept);
+        assert(probe.ref_count == 2);
+        series.remove_first_where(p => p == probe);
+        assert(probe.ref_count == 1);
+        assert(kept.ref_count == 2);
+        series.remove_all_where(p => true);
+        assert(kept.ref_count == 1);
+        assert(series.count() == 0);
+    });
+
+    Test.add_func("/invercargill/structure/series/clear_releases_ownership", () => {
+        var probe = new OwnershipProbe();
+        var series = new Series<OwnershipProbe>();
+        series.add(probe);
+        series.add(probe);
+        assert(probe.ref_count == 3);
+        series.clear();
+        assert(probe.ref_count == 1);
+    });
 }

+ 3 - 0
src/tests/TestRunner.vala

@@ -1,4 +1,7 @@
 
+public class OwnershipProbe : Object {
+}
+
 public static int main(string[] args) {
 
     Test.init(ref args);