Rewrite ownership to use faction_id to avoid pointer crashes (#34217)

* first attempt to change ownership to id instead of pointer to faction
* changed vehicle ownership to faction_id instead of pointers
* ensure get_faction() wont return nullptr, by returning "no_faction" if my_fac is not valid
* fix vehicle deserializing owner from older saves

src/activity_item_handling.cpp

@@ -735,8 +735,10 @@ static void move_items( player &p, const tripoint &relative_dest, bool to_vehicl
         if( !newit.made_of_from_type( LIQUID ) ) {
             // This is for hauling across zlevels, remove when going up and down stairs
             // is no longer teleportation
-            if( !newit.has_owner() && p.is_player() ) {
-                newit.set_owner( p.get_faction() );
+            if( newit.is_owned_by( p, true ) ) {
+                newit.set_owner( p );
+            } else {
+                continue;
             }
             const tripoint src = target.position();
             int distance = src.z == dest.z ? std::max( rl_dist( src, dest ), 1 ) : 1;

src/advanced_inv.cpp

@@ -353,12 +353,9 @@ void advanced_inventory::print_items( advanced_inventory_pane &pane, bool active
         std::string item_name;
         std::string stolen_string;
         bool stolen = false;
-        if( it.has_owner() ) {
-            const faction *item_fac = it.get_owner();
-            if( item_fac != g->u.get_faction() ) {
-                stolen_string = "<color_light_red>!</color>";
-                stolen = true;
-            }
+        if( !it.is_owned_by( g->u, true ) ) {
+            stolen_string = "<color_light_red>!</color>";
+            stolen = true;
         }
         if( it.is_money() ) {
             //Count charges

src/character.cpp

@@ -3976,7 +3976,7 @@ void Character::healed_bp( int bp, int amount )
 
 void Character::set_fac_id( const std::string &my_fac_id )
 {
-    fac_id = string_id<faction>( my_fac_id );
+    fac_id = faction_id( my_fac_id );
 }
 
 std::string get_stat_name( Character::stat Stat )

src/character.h

@@ -1077,7 +1077,7 @@ class Character : public Creature, public visitable<Character>
         // 2 - allies are in your_followers faction; NPCATT_FOLLOW is follower but not an ally
         // 0 - allies may be in your_followers faction; NPCATT_FOLLOW is an ally (legacy)
         int faction_api_version = 2;  // faction API versioning
-        string_id<faction> fac_id; // A temp variable used to inform the game which faction to link
+        faction_id fac_id; // A temp variable used to inform the game which faction to link
         faction *my_fac = nullptr;
 
     private:

src/condition.cpp

@@ -779,10 +779,8 @@ void conditional_t<T>::set_has_stolen_item( bool is_npc )
         npc &p = *d.beta;
         bool found_in_inv = false;
         for( auto &elem : actor->inv_dump() ) {
-            if( elem->get_old_owner() ) {
-                if( elem->get_old_owner() == p.get_faction() ) {
-                    found_in_inv = true;
-                }
+            if( elem->is_old_owner( p, true ) ) {
+                found_in_inv = true;
             }
         }
         return found_in_inv;

src/crafting.cpp

@@ -630,7 +630,7 @@ static void finalize_crafted_item( item &newit, faction *maker_fac )
     }
     // TODO for now this assumes player is doing the crafting
     // this will need to be updated when NPCs do crafting
-    newit.set_owner( maker_fac );
+    newit.set_owner( maker_fac->id );
 }
 
 static cata::optional<item_location> wield_craft( player &p, item &craft )

src/faction.cpp

@@ -386,6 +386,9 @@ faction *faction_manager::add_new_faction( const std::string &name_new, const fa
 
 faction *faction_manager::get( const faction_id &id, const bool complain )
 {
+    if( id.is_null() ) {
+        return get( faction_id( "no_faction" ) );
+    }
     for( auto &elem : factions ) {
         if( elem.first == id ) {
             if( !elem.second.validated ) {

src/faction_camp.cpp

@@ -3584,7 +3584,7 @@ void apply_camp_ownership( const tripoint &camp_pos, int radius )
             camp_pos + point( radius, radius ) ) ) {
         auto items = g->m.i_at( p.xy() );
         for( item &elem : items ) {
-            elem.set_owner( g->u.get_faction() );
+            elem.set_owner( g->u );
         }
     }
 }

src/game.cpp

@@ -843,7 +843,7 @@ bool game::start_game()
         }
     }
     for( auto &e : u.inv_dump() ) {
-        e->set_owner( g->u.get_faction() );
+        e->set_owner( g->u );
     }
     // Now that we're done handling coordinates, ensure the player's submap is in the center of the map
     update_map( u );
@@ -969,6 +969,7 @@ void game::create_starting_npcs()
     tmp->mission = NPC_MISSION_SHELTER;
     tmp->chatbin.first_topic = "TALK_SHELTER";
     tmp->toggle_trait( trait_id( "NPC_STARTING_NPC" ) );
+    tmp->set_fac( faction_id( "no_faction" ) );
     //One random starting NPC mission
     tmp->add_new_mission( mission::reserve_random( ORIGIN_OPENER_NPC, tmp->global_omt_location(),
                           tmp->getID() ) );
@@ -2690,7 +2691,7 @@ void game::load( const save_t &name )
     validate_camps();
     update_map( u );
     for( auto &e : u.inv_dump() ) {
-        e->set_owner( g->u.get_faction() );
+        e->set_owner( g->u );
     }
     // legacy, needs to be here as we access the map.
     if( !u.getID().is_valid() ) {

src/inventory.cpp

@@ -432,7 +432,7 @@ void inventory::form_from_map( map &m, const tripoint &origin, int range, const
             for( auto &i : m.i_at( p ) ) {
                 // if its *the* player requesting this from from map inventory
                 // then dont allow items owned by another faction to be factored into recipe components etc.
-                if( pl && i.has_owner() && i.get_owner() != pl->get_faction() ) {
+                if( pl && !i.is_owned_by( *pl, true ) ) {
                     continue;
                 }
                 if( !i.made_of( LIQUID ) ) {

src/item.cpp

@@ -1044,6 +1044,61 @@ static int get_base_env_resist( const item &it )
 
 }
 
+bool item::is_owned_by( const Character &c, bool available_to_take ) const
+{
+    // owner.is_null() implies faction_id( "no_faction" ) which shouldnt happen, or no owner at all.
+    // either way, certain situations this means the thing is available to take.
+    // in other scenarios we actaully really want to check for id == id, even for no_faction
+    if( owner.is_null() ) {
+        return available_to_take;
+    }
+    if( !c.get_faction() ) {
+        debugmsg( "Character %s has no faction", c.disp_name() );
+        return false;
+    }
+    return c.get_faction()->id == get_owner();
+}
+
+bool item::is_old_owner( const Character &c, bool available_to_take ) const
+{
+    if( old_owner.is_null() ) {
+        return available_to_take;
+    }
+    if( !c.get_faction() ) {
+        debugmsg( "Character %s has no faction.", c.disp_name() );
+        return false;
+    }
+    return c.get_faction()->id == get_old_owner();
+}
+
+std::string item::get_owner_name() const
+{
+    if( !g->faction_manager_ptr->get( owner ) ) {
+        debugmsg( "item::get_owner_name() item %s has no valid nor null faction id ", tname() );
+        return "no owner";
+    }
+    return g->faction_manager_ptr->get( owner )->name;
+}
+
+void item::set_owner( const Character &c )
+{
+    if( !c.get_faction() ) {
+        debugmsg( "item::set_owner() Character %s has no valid faction", c.disp_name() );
+        return;
+    }
+    owner = c.get_faction()->id;
+}
+
+faction_id item::get_owner() const
+{
+    return owner;
+}
+
+faction_id item::get_old_owner() const
+{
+    return old_owner;
+}
+
 std::string item::info( std::vector<iteminfo> &info, const iteminfo_query *parts, int batch ) const
 {
     std::stringstream temp1;
@@ -1177,8 +1232,8 @@ std::string item::info( std::vector<iteminfo> &info, const iteminfo_query *parts
             }, enumeration_conjunction::none );
             info.push_back( iteminfo( "BASE", string_format( _( "Material: %s" ), material_list ) ) );
         }
-        if( has_owner() ) {
-            info.push_back( iteminfo( "BASE", string_format( _( "Owner: %s" ), _( get_owner()->name ) ) ) );
+        if( !owner.is_null() ) {
+            info.push_back( iteminfo( "BASE", string_format( _( "Owner: %s" ), _( get_owner_name() ) ) ) );
         }
         if( has_var( "contained_name" ) && parts->test( iteminfo_parts::BASE_CONTENTS ) ) {
             info.push_back( iteminfo( "BASE", string_format( _( "Contains: %s" ),
@@ -3294,17 +3349,18 @@ void item::on_wield( player &p, int mv )
 
 void item::handle_pickup_ownership( Character &c )
 {
-    faction *yours = c.get_faction();
+    if( is_owned_by( c ) ) {
+        return;
+    }
     // Add ownership to item if unowned
-    if( !has_owner() ) {
-        set_owner( yours );
+    if( owner.is_null() ) {
+        set_owner( c );
     } else {
-        if( get_owner() != yours && &c == &g->u ) {
+        if( !is_owned_by( c ) && &c == &g->u ) {
             std::vector<npc *> witnesses;
             for( npc &elem : g->all_npcs() ) {
                 if( rl_dist( elem.pos(), g->u.pos() ) < MAX_VIEW_DISTANCE && elem.get_faction() &&
-                    elem.get_faction()->id != faction_id( "no_faction" ) &&
-                    elem.get_faction() == get_owner() && elem.sees( g->u.pos() ) ) {
+                    is_owned_by( elem ) && elem.sees( g->u.pos() ) ) {
                     elem.say( "<witnessed_thievery>", 7 );
                     npc *npc_to_add = &elem;
                     witnesses.push_back( npc_to_add );
@@ -3325,7 +3381,7 @@ void item::handle_pickup_ownership( Character &c )
                     witnesses[random_index]->witness_thievery( &*this );
                 }
             }
-            set_owner( yours );
+            set_owner( c );
         }
     }
 }

src/item.h

@@ -58,6 +58,7 @@ struct itype;
 struct islot_comestible;
 
 using bodytype_id = std::string;
+using faction_id = string_id<faction>;
 struct islot_armor;
 struct use_function;
 class item_category;
@@ -1906,33 +1907,24 @@ class item : public visitable<item>
         void set_birthday( const time_point &bday );
         void handle_pickup_ownership( Character &c );
         int get_gun_ups_drain() const;
-        inline void set_old_owner( const faction *temp_owner ) {
+        inline void set_old_owner( const faction_id temp_owner ) {
             old_owner = temp_owner;
         }
         inline void remove_old_owner() {
-            old_owner = nullptr;
+            old_owner = faction_id::NULL_ID();
         }
-        inline void set_owner( faction *new_owner ) {
+        inline void set_owner( const faction_id new_owner ) {
             owner = new_owner;
         }
+        void set_owner( const Character &c );
         inline void remove_owner() {
-            owner = nullptr;
-        }
-        inline const faction *get_owner() const {
-            if( owner ) {
-                return owner;
-            }
-            return nullptr;
-        }
-        inline const faction *get_old_owner() const {
-            if( old_owner ) {
-                return old_owner;
-            }
-            return nullptr;
-        }
-        inline bool has_owner() const {
-            return owner;
+            owner = faction_id::NULL_ID();
         }
+        faction_id get_owner() const;
+        faction_id get_old_owner() const;
+        bool is_owned_by( const Character &c, bool available_to_take = false ) const;
+        bool is_old_owner( const Character &c, bool available_to_take = false ) const;
+        std::string get_owner_name() const;
         int get_min_str() const;
 
         const cata::optional<islot_comestible> &get_comestible() const;
@@ -2097,9 +2089,9 @@ class item : public visitable<item>
          */
         phase_id current_phase = static_cast<phase_id>( 0 );
         // The faction that owns this item.
-        const faction *owner = nullptr;
+        faction_id owner = faction_id::NULL_ID();
         // The faction that previously owned this item
-        const faction *old_owner = nullptr;
+        faction_id old_owner = faction_id::NULL_ID();
         int damage_ = 0;
         light_emission light = nolight;
 

src/iuse.cpp

@@ -5694,7 +5694,7 @@ int iuse::unfold_generic( player *p, item *it, bool, const tripoint & )
     } else {
         unfold_msg = _( unfold_msg );
     }
-    veh->set_owner( g->u.get_faction() );
+    veh->set_owner( *p );
     p->add_msg_if_player( m_neutral, unfold_msg, veh->name );
 
     p->moves -= it->get_var( "moves", to_turns<int>( 5_seconds ) );

src/iuse_actor.cpp

@@ -542,7 +542,7 @@ int unfold_vehicle_iuse::use( player &p, item &it, bool, const tripoint & ) cons
         p.add_msg_if_player( m_info, _( "There's no room to unfold the %s." ), it.tname() );
         return 0;
     }
-    veh->set_owner( g->u.get_faction() );
+    veh->set_owner( p );
 
     // Mark the vehicle as foldable.
     veh->tags.insert( "convertible" );

src/mapgen.cpp

@@ -6587,17 +6587,16 @@ character_id map::place_npc( const point &p, const string_id<npc_template> &type
 void map::apply_faction_ownership( const point &p1, const point &p2,
                                    const faction_id id )
 {
-    faction *fac = g->faction_manager_ptr->get( id );
     for( const tripoint &p : points_in_rectangle( tripoint( p1, abs_sub.z ), tripoint( p2,
             abs_sub.z ) ) ) {
         auto items = i_at( p.xy() );
         for( item &elem : items ) {
-            elem.set_owner( fac );
+            elem.set_owner( id );
         }
         vehicle *source_veh = veh_pointer_or_null( veh_at( p ) );
         if( source_veh ) {
             if( !source_veh->has_owner() ) {
-                source_veh->set_owner( fac );
+                source_veh->set_owner( id );
             }
         }
     }