Prevent use of stale pointers in item_location

src/item.cpp

@@ -238,6 +238,11 @@ item::item( const itype *type, time_point turn, solitary_tag )
 item::item( const itype_id &id, time_point turn, solitary_tag tag )
     : item( find_type( id ), turn, tag ) {}
 
+safe_reference<item> item::get_safe_reference()
+{
+    return anchor.reference_to( this );
+}
+
 static const item *get_most_rotten_component( const item &craft )
 {
     const item *most_rotten = nullptr;

src/item.h

@@ -22,6 +22,7 @@
 #include "io_tags.h"
 #include "item_location.h"
 #include "requirements.h"
+#include "safe_reference.h"
 #include "string_id.h"
 #include "type_id.h"
 #include "units.h"
@@ -184,6 +185,10 @@ class item : public visitable<item>
         /** For constructing in-progress crafts */
         item( const recipe *rec, int qty, std::list<item> items, std::vector<item_comp> selections );
 
+        /** Return a pointer-like type that's automatically invalidated if this
+         * item is destroyed or assigned-to */
+        safe_reference<item> get_safe_reference();
+
         /**
          * Filter converting this instance to another type preserving all other aspects
          * @param new_type the type id to convert to
@@ -1983,6 +1988,7 @@ class item : public visitable<item>
         cata::flat_set<std::string> item_tags; // generic item specific flags
 
     private:
+        safe_reference_anchor anchor;
         const itype *curammo = nullptr;
         std::map<std::string, std::string> item_vars;
         const mtype *corpse = nullptr;

src/item_location.cpp

@@ -67,64 +67,81 @@ class item_location::impl
         class item_on_vehicle;
 
         impl() = default;
-        impl( item *what ) : what( what ) {}
-        impl( int idx ) : idx( idx ) {}
+        impl( item *i ) : what( i->get_safe_reference() ), needs_unpacking( false ) {}
+        impl( int idx ) : idx( idx ), needs_unpacking( true ) {}
 
         virtual ~impl() = default;
 
-        virtual bool valid() const {
-            return false;
+        virtual type where() const = 0;
+        virtual tripoint position() const = 0;
+        virtual std::string describe( const Character * ) const = 0;
+        virtual int obtain( Character &, int ) = 0;
+        virtual int obtain_cost( const Character &, int ) const = 0;
+        virtual void remove_item() = 0;
+        virtual void serialize( JsonOut &js ) const = 0;
+        virtual item *unpack( int ) const = 0;
+
+        item *target() const {
+            ensure_unpacked();
+            return what.get();
+        }
+
+        bool valid() const {
+            ensure_unpacked();
+            return !!what;
+        }
+    private:
+        void ensure_unpacked() const {
+            if( needs_unpacking ) {
+                what = unpack( idx )->get_safe_reference();
+                needs_unpacking = false;
+            }
         }
+        mutable safe_reference<item> what;
+        mutable int idx = -1;
+        mutable bool needs_unpacking;
+
+    public:
+        //Flag that controls whether functions like obtain() should stack the obtained item
+        //with similar existing items in the inventory or create a new stack for the item
+        bool should_stack = true;
+};
 
-        virtual type where() const {
+class item_location::impl::nowhere : public item_location::impl
+{
+    public:
+        type where() const override {
             return type::invalid;
         }
 
-        virtual tripoint position() const {
+        tripoint position() const override {
+            debugmsg( "invalid use of nowhere item_location" );
             return tripoint_min;
         }
 
-        virtual std::string describe( const Character * ) const {
+        std::string describe( const Character * ) const override {
+            debugmsg( "invalid use of nowhere item_location" );
             return "";
         }
 
-        virtual int obtain( Character &, int ) {
+        int obtain( Character &, int ) override {
+            debugmsg( "invalid use of nowhere item_location" );
             return INT_MIN;
         }
 
-        virtual int obtain_cost( const Character &, int ) const {
+        int obtain_cost( const Character &, int ) const override {
+            debugmsg( "invalid use of nowhere item_location" );
             return 0;
         }
 
-        virtual void remove_item() {}
-
-        virtual void serialize( JsonOut &js ) const = 0;
-
-        virtual item *unpack( int ) const {
-            return nullptr;
+        void remove_item() override {
+            debugmsg( "invalid use of nowhere item_location" );
         }
 
-        item *target() const {
-            if( what == nullptr ) {
-                what = unpack( idx );
-            }
-            return what;
+        item *unpack( int ) const override {
+            return nullptr;
         }
 
-    private:
-        mutable item *what = nullptr;
-        mutable int idx = -1;
-        //Only used for stacked cash card currently, needed to be able to process a stack of different items
-
-    public:
-        //Flag that controls whether functions like obtain() should stack the obtained item
-        //with similar existing items in the inventory or create a new stack for the item
-        bool should_stack = true;
-};
-
-class item_location::impl::nowhere : public item_location::impl
-{
-    public:
         void serialize( JsonOut &js ) const override {
             js.start_object();
             js.member( "type", "null" );
@@ -141,10 +158,6 @@ class item_location::impl::item_on_map : public item_location::impl
         item_on_map( const map_cursor &cur, item *which ) : impl( which ), cur( cur ) {}
         item_on_map( const map_cursor &cur, int idx ) : impl( idx ), cur( cur ) {}
 
-        bool valid() const override {
-            return target() && cur.has_item( *target() );
-        }
-
         void serialize( JsonOut &js ) const override {
             js.start_object();
             js.member( "type", "map" );
@@ -210,20 +223,6 @@ class item_location::impl::item_on_map : public item_location::impl
         }
 };
 
-static bool gun_has_item( const item &gun, const item *it )
-{
-    if( !gun.is_gun() ) {
-        return false;
-    }
-
-    if( gun.magazine_current() == it ) {
-        return true;
-    }
-
-    auto gms = gun.gunmods();
-    return !gms.empty() && std::find( gms.begin(), gms.end(), it ) != gms.end();
-}
-
 class item_location::impl::item_on_person : public item_location::impl
 {
     private:
@@ -233,13 +232,6 @@ class item_location::impl::item_on_person : public item_location::impl
         item_on_person( Character &who, item *which ) : impl( which ), who( who ) {}
         item_on_person( Character &who, int idx ) : impl( idx ), who( who ) {}
 
-        bool valid() const override {
-            const item *targ = target();
-            return targ && who.has_item_with( [targ]( const item & it ) {
-                return &it == targ || gun_has_item( it, targ );
-            } );
-        }
-
         void serialize( JsonOut &js ) const override {
             js.start_object();
             js.member( "type", "character" );
@@ -359,19 +351,6 @@ class item_location::impl::item_on_vehicle : public item_location::impl
         item_on_vehicle( const vehicle_cursor &cur, item *which ) : impl( which ), cur( cur ) {}
         item_on_vehicle( const vehicle_cursor &cur, int idx ) : impl( idx ), cur( cur ) {}
 
-        bool valid() const override {
-            if( !target() ) {
-                return false;
-            }
-            if( &cur.veh.parts[ cur.part ].base == target() ) {
-                return true; // vehicle_part::base
-            }
-            if( cur.has_item( *target() ) ) {
-                return true; // item within CARGO
-            }
-            return false;
-        }
-
         void serialize( JsonOut &js ) const override {
             js.start_object();
             js.member( "type", "vehicle" );

src/safe_reference.cpp

@@ -0,0 +1,16 @@
+#include "safe_reference.h"
+
+safe_reference_anchor::safe_reference_anchor()
+{
+    impl = std::make_shared<empty>();
+}
+
+safe_reference_anchor::safe_reference_anchor( const safe_reference_anchor & ) :
+    safe_reference_anchor()
+{}
+
+safe_reference_anchor &safe_reference_anchor::operator=( const safe_reference_anchor & )
+{
+    impl = std::make_shared<empty>();
+    return *this;
+}

src/safe_reference.h

@@ -0,0 +1,73 @@
+#pragma once
+#ifndef CATA_SAFE_REFERENCE_H
+#define CATA_SAFE_REFERENCE_H
+
+/**
+A pair of classes to provide safe references to objects.
+
+A safe_reference_anchor is made a member of some object, and a safe_reference
+to that object (or any other object with the same lifetime) can be obtained
+from the safe_reference_anchor.
+
+When the safe_reference is destroyed or assigned-to, all safe_references
+derived from it are invalidated.  When one attempts to fetch the referenced
+object from an invalidated safe_reference, it returns nullptr.
+
+The motivating use case is to store references to items in item_locations in a
+way that is safe if that item is moved or destroyed.
+
+*/
+
+#include <memory>
+
+template<typename T>
+class safe_reference
+{
+    public:
+        safe_reference() = default;
+
+        T *get() const {
+            return impl.lock().get();
+        }
+
+        explicit operator bool() const {
+            return !!*this;
+        }
+
+        bool operator!() const {
+            return impl.expired();
+        }
+
+        T &operator*() const {
+            return *get();
+        }
+
+        T *operator->() const {
+            return get();
+        }
+    private:
+        friend class safe_reference_anchor;
+
+        safe_reference( const std::shared_ptr<T> &p ) : impl( p ) {}
+
+        std::weak_ptr<T> impl;
+};
+
+class safe_reference_anchor
+{
+    public:
+        safe_reference_anchor();
+        safe_reference_anchor( const safe_reference_anchor & );
+        safe_reference_anchor &operator=( const safe_reference_anchor & );
+
+        template<typename T>
+        safe_reference<T> reference_to( T *object ) {
+            // Using the shared_ptr aliasing constructor
+            return safe_reference<T>( std::shared_ptr<T>( impl, object ) );
+        }
+    private:
+        struct empty {};
+        std::shared_ptr<empty> impl;
+};
+
+#endif // CATA_SAFE_REFERENCE_H

tests/item_location_test.cpp

@@ -0,0 +1,57 @@
+#include "catch/catch.hpp"
+#include "game.h"
+#include "item.h"
+#include "map_helpers.h"
+#include "rng.h"
+#include "stringmaker.h"
+
+TEST_CASE( "item_location_can_maintain_reference_despite_item_removal", "[item][item_location]" )
+{
+    clear_map();
+    map &m = g->m;
+    tripoint pos( 60, 60, 0 );
+    m.i_clear( pos );
+    m.add_item( pos, item( "jeans" ) );
+    m.add_item( pos, item( "jeans" ) );
+    m.add_item( pos, item( "tshirt" ) );
+    m.add_item( pos, item( "jeans" ) );
+    m.add_item( pos, item( "jeans" ) );
+    map_cursor cursor( pos );
+    item *tshirt = nullptr;
+    cursor.visit_items( [&tshirt]( item * i ) {
+        if( i->typeId() == "tshirt" ) {
+            tshirt = i;
+            return VisitResponse::ABORT;
+        }
+        return VisitResponse::NEXT;
+    } );
+    REQUIRE( tshirt != nullptr );
+    item_location item_loc( cursor, tshirt );
+    REQUIRE( item_loc->typeId() == "tshirt" );
+    for( int j = 0; j < 4; ++j ) {
+        // Delete up to 4 random jeans
+        map_stack stack = m.i_at( pos );
+        REQUIRE( !stack.empty() );
+        item *i = &random_entry_opt( stack )->get();
+        if( i->typeId() == "jeans" ) {
+            m.i_rem( pos, i );
+        }
+    }
+    CAPTURE( m.i_at( pos ) );
+    REQUIRE( item_loc );
+    CHECK( item_loc->typeId() == "tshirt" );
+}
+
+TEST_CASE( "item_location_doesnt_return_stale_map_item", "[item][item_location]" )
+{
+    clear_map();
+    map &m = g->m;
+    tripoint pos( 60, 60, 0 );
+    m.i_clear( pos );
+    m.add_item( pos, item( "tshirt" ) );
+    item_location item_loc( map_cursor( pos ), &m.i_at( pos ).only_item() );
+    REQUIRE( item_loc->typeId() == "tshirt" );
+    m.i_rem( pos, &*item_loc );
+    m.add_item( pos, item( "jeans" ) );
+    CHECK( !item_loc );
+}