[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

@@ -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,17 +93,25 @@ 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)) {
                 FactPair effect_pair = effect.get_fact().get_pair();
                 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);
     }
 }
 

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.