From c0ca355e55c10f6f8911430d595d638c9162d64e Mon Sep 17 00:00:00 2001
From: Silvan Sievers <silvan.sievers@unibas.ch>
Date: Mon, 8 Jan 2024 12:09:16 +0100
Subject: [PATCH] [issue1115] Fix a bug in a state registry.

When copying a state, the previous code would use a dangling pointer to
access state values of the copied state. This issue fixes this bug by
looking up the the correct state values.
---
 src/search/state_registry.cc | 24 ++++++++++++++++++++++--
 src/search/state_registry.h  |  7 +++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/src/search/state_registry.cc b/src/search/state_registry.cc
index e7d882cb5a..22a874856b 100644
--- a/src/search/state_registry.cc
+++ b/src/search/state_registry.cc
@@ -41,6 +41,12 @@ State StateRegistry::lookup_state(StateID id) const {
     return task_proxy.create_state(*this, id, buffer);
 }
 
+State StateRegistry::lookup_state(
+    StateID id, vector<int> &&state_values) const {
+    const PackedStateBin *buffer = state_data_pool[id.value];
+    return task_proxy.create_state(*this, id, buffer, move(state_values));
+}
+
 const State &StateRegistry::get_initial_state() {
     if (!cached_initial_state) {
         int num_bins = get_bins_per_state();
@@ -64,6 +70,12 @@ const State &StateRegistry::get_initial_state() {
 //     operating on state buffers (PackedStateBin *).
 State StateRegistry::get_successor_state(const State &predecessor, const OperatorProxy &op) {
     assert(!op.is_axiom());
+    /*
+      TODO: ideally, we would not modify state_data_pool here and in
+      insert_id_or_pop_state, but only at one place, to avoid errors like
+      buffer becoming a dangling pointer. This used to be a bug before being
+      fixed in https://issues.fast-downward.org/issue1115.
+    */
     state_data_pool.push_back(predecessor.get_buffer());
     PackedStateBin *buffer = state_data_pool[state_data_pool.size() - 1];
     /* Experiments for issue348 showed that for tasks with axioms it's faster
@@ -81,8 +93,12 @@ State StateRegistry::get_successor_state(const State &predecessor, const Operato
         for (size_t i = 0; i < new_values.size(); ++i) {
             state_packer.set(buffer, i, new_values[i]);
         }
+        /*
+          NOTE: insert_id_or_pop_state possibly invalidates buffer, hence
+          we use lookup_state to retrieve the state using the correct buffer.
+        */
         StateID id = insert_id_or_pop_state();
-        return task_proxy.create_state(*this, id, buffer, move(new_values));
+        return lookup_state(id, move(new_values));
     } else {
         for (EffectProxy effect : op.get_effects()) {
             if (does_fire(effect, predecessor)) {
@@ -90,8 +106,12 @@ State StateRegistry::get_successor_state(const State &predecessor, const Operato
                 state_packer.set(buffer, effect_pair.var, effect_pair.value);
             }
         }
+        /*
+          NOTE: insert_id_or_pop_state possibly invalidates buffer, hence
+          we use lookup_state to retrieve the state using the correct buffer.
+        */
         StateID id = insert_id_or_pop_state();
-        return task_proxy.create_state(*this, id, buffer);
+        return lookup_state(id);
     }
 }
 
diff --git a/src/search/state_registry.h b/src/search/state_registry.h
index bb35af642b..0604b0fff5 100644
--- a/src/search/state_registry.h
+++ b/src/search/state_registry.h
@@ -190,6 +190,13 @@ class StateRegistry : public subscriber::SubscriberService<StateRegistry> {
     */
     State lookup_state(StateID id) const;
 
+    /*
+      Like lookup_state above, but creates a state with unpacked data,
+      moved in via state_values. It is the caller's responsibility that
+      the unpacked data matches the state's data.
+    */
+    State lookup_state(StateID id, std::vector<int> &&state_values) const;
+
     /*
       Returns a reference to the initial state and registers it if this was not
       done before. The result is cached internally so subsequent calls are cheap.