Stop storing players as bare pointers - use unique_ptr instead

Should fix some potential memory leaks that were caught by static analysis

src/universe/party.cpp

@@ -40,18 +40,11 @@ cParty::cParty(ePartyPreset party_preset) {
 	for(int i = 0; i < 10; i++)
 		out_c[i].exists = false;
 	for(int i = 0; i < 6; i++)
-		adven[i] = new cPlayer(*this, party_preset, i);
+		adven[i].reset(new cPlayer(*this, party_preset, i));
 	for(auto& monst : imprisoned_monst)
 		monst = 0;
 }
 
-cParty::~cParty() {
-	for(int i = 0; i < 6; i++) {
-		delete adven[i];
-		adven[i] = nullptr;
-	}
-}
-
 cParty::cParty(const cParty& other)
 	: iLiving(other)
 	, next_pc_id(other.next_pc_id)
@@ -109,8 +102,7 @@ cParty::cParty(const cParty& other)
 	memcpy(stuff_done, other.stuff_done, sizeof(stuff_done));
 	memcpy(setup, other.setup, sizeof(setup));
 	for(int i = 0; i < 6; i++) {
-		adven[i] = new cPlayer(*other.adven[i]);
-		adven[i]->join_party(*this);
+		adven[i].reset(new cPlayer(*this, *other.adven[i]));
 	}
 }
 
@@ -184,12 +176,8 @@ void cParty::swap(cParty& other) {
 	memcpy(temp_setup, setup, sizeof(setup));
 	memcpy(setup, other.setup, sizeof(setup));
 	memcpy(other.setup, temp_setup, sizeof(setup));
-	// Fixup the references of PCs to their party
 	for(size_t i = 0; i < adven.size(); i++) {
-		adven[i]->join_party(*this);
-	}
-	for(size_t i = 0; i < other.adven.size(); i++) {
-		other.adven[i]->join_party(other);
+		std::swap(adven[i], other.adven[i]);
 	}
 }
 
@@ -351,22 +339,27 @@ void cParty::cEncNote::import_legacy(int16_t(& old)[2], const cScenario& scenari
 
 void cParty::import_legacy(legacy::pc_record_type(& old)[6]) {
 	for(int i = 0; i < 6; i++) {
-		delete adven[i];
-		adven[i] = new cPlayer(*this);
+		adven[i].reset(new cPlayer(*this));
 		adven[i]->import_legacy(old[i]);
 	}
 }
 
+std::unique_ptr<cPlayer> cParty::remove_pc(size_t spot) {
+	if(spot >= 6) return nullptr;
+	adven[spot]->party = nullptr;
+	return std::move(adven[spot]);
+}
+
 void cParty::new_pc(size_t spot) {
-	replace_pc(spot, new cPlayer(*this));
+	std::unique_ptr<cPlayer> new_pc{new cPlayer(*this)};
+	replace_pc(spot, std::move(new_pc));
 	adven[spot]->main_status = eMainStatus::ALIVE;
 }
 
-void cParty::replace_pc(size_t spot, cPlayer* with) {
+void cParty::replace_pc(size_t spot, std::unique_ptr<cPlayer> with) {
 	if(spot < 6 && with != nullptr) {
-		with->join_party(*this);
-		delete adven[spot];
-		adven[spot] = with;
+		with->party = this;
+		adven[spot] = std::move(with);
 	}
 }
 

src/universe/party.hpp

@@ -123,7 +123,7 @@ public:
 	long long total_m_killed, total_dam_done, total_xp_gained, total_dam_taken;
 	std::string scen_name;
 private:
-	std::array<cPlayer*,6> adven;
+	std::array<std::unique_ptr<cPlayer>,6> adven;
 public:
 	unsigned short setup[4][64][64]; // formerly setup_save_type
 	std::array<std::vector<cItem>,3> stored_items; // formerly stored_items_list_type
@@ -178,8 +178,9 @@ public:
 	int get_level() const;
 	int calc_day() const;
 	
+	std::unique_ptr<cPlayer> remove_pc(size_t spot);
 	void new_pc(size_t spot);
-	void replace_pc(size_t spot, cPlayer* with);
+	void replace_pc(size_t spot, std::unique_ptr<cPlayer> with);
 	size_t free_space();
 	size_t count(eMainStatus type = eMainStatus::ALIVE);
 	void void_pcs();
@@ -219,7 +220,6 @@ public:
 	typedef std::vector<cJournal>::iterator journalIter;
 	typedef std::vector<cConvers>::iterator talkIter;
 	cParty(ePartyPreset party_preset = PARTY_DEFAULT);
-	~cParty();
 	// Copy-and-swap
 	void swap(cParty& other);
 	cParty(const cParty& other);

src/universe/pc.cpp

@@ -943,7 +943,12 @@ void cPlayer::finish_create() {
 	}
 }
 
-cPlayer::cPlayer(cParty& party) : party(&party), weap_poisoned(*this) {
+cPlayer::cPlayer(cParty& party) : cPlayer(no_party) {
+	this->party = &party;
+	unique_id = party.next_pc_id++;
+}
+
+cPlayer::cPlayer(no_party_t) : weap_poisoned(*this) {
 	main_status = eMainStatus::ABSENT;
 	name = "\n";
 	
@@ -967,8 +972,6 @@ cPlayer::cPlayer(cParty& party) : party(&party), weap_poisoned(*this) {
 	race = eRace::HUMAN;
 	direction = DIR_N;
 	combat_pos = {-1,-1};
-	
-	unique_id = party.next_pc_id++;
 }
 
 cPlayer::cPlayer(cParty& party,ePartyPreset key,short slot) : cPlayer(party) {
@@ -1115,9 +1118,13 @@ cPlayer::cPlayer(cParty& party,ePartyPreset key,short slot) : cPlayer(party) {
 	}
 }
 
-cPlayer::cPlayer(const cPlayer& other)
+cPlayer::cPlayer(cParty& party, const cPlayer& other) : cPlayer(no_party, other) {
+	this->party = &party;
+	unique_id = party.next_pc_id++;
+}
+
+cPlayer::cPlayer(no_party_t, const cPlayer& other)
 	: iLiving(other)
-	, party(other.party)
 	, main_status(other.main_status)
 	, name(other.name)
 	, skills(other.skills)
@@ -1136,7 +1143,6 @@ cPlayer::cPlayer(const cPlayer& other)
 	, weap_poisoned(*this, other.weap_poisoned.slot)
 	, traits(other.traits)
 	, race(other.race)
-	, unique_id(party->next_pc_id++)
 	, last_cast(other.last_cast)
 	, combat_pos(other.combat_pos)
 	, parry(other.parry)
@@ -1148,7 +1154,7 @@ cPlayer::cPlayer(cPlayer&& other) : weap_poisoned(*this, other.weap_poisoned.slo
 }
 
 void cPlayer::swap(cPlayer& other) {
-	std::swap(party, other.party);
+	// Don't swap the party reference!
 	std::swap(main_status, other.main_status);
 	std::swap(name, other.name);
 	std::swap(skills, other.skills);

src/universe/pc.hpp

@@ -26,6 +26,8 @@
 
 namespace legacy { struct pc_record_type; };
 
+static struct no_party_t {} no_party;
+
 enum class eBuyStatus {OK, NO_SPACE, NEED_GOLD, TOO_HEAVY, HAVE_LOTS};
 
 enum ePartyPreset {PARTY_BLANK, PARTY_DEFAULT, PARTY_DEBUG};
@@ -67,9 +69,13 @@ inline bool operator!=(const cInvenSlot& a, const cInvenSlot& b) {
 }
 
 class cPlayer : public iLiving {
-	cParty* party;
+	cParty* party = nullptr;
 	template<typename Fcn>
 	cInvenSlot find_item_matching(Fcn fcn);
+	// Allow the party to overwrite the PC's party reference
+	// It only does this (and should ever do this) in remove_pc and replace_pc!
+	// Sadly there is no good way to friend just those two functions.
+	friend class cParty;
 	static const int INVENTORY_SIZE = 24;
 public:
 	// A nice convenient bitset with just the low 30 bits set, for initializing spells
@@ -166,19 +172,19 @@ public:
 	short stat_adj(eSkill skill) const;
 	eBuyStatus ok_to_buy(short cost,cItem item) const;
 	
-	void join_party(cParty& p) {party = &p;}
-	cPlayer* leave_party() {party = nullptr; return this;}
-	
 	void import_legacy(legacy::pc_record_type old);
+	cPlayer(no_party_t);
 	cPlayer(cParty& party);
 	cPlayer(cParty& party,ePartyPreset key,short slot);
+	cPlayer(no_party_t, const cPlayer& other);
+	cPlayer(cParty& party, const cPlayer& other);
 	short get_tnl();
 	void writeTo(std::ostream& file) const;
 	void readFrom(std::istream& file);
 	virtual ~cPlayer() = default;
 	// Copy-and-swap
 	void swap(cPlayer& other);
-	cPlayer(const cPlayer& other);
+	cPlayer(const cPlayer& other) = delete;
 	cPlayer(cPlayer&& other);
 	// For now, not assignable because of an issue of how to handle the unique_id
 	cPlayer& operator=(cPlayer other) = delete;

src/universe/universe.cpp

@@ -912,7 +912,6 @@ cUniverse::cUniverse(const cUniverse& other)
 	, cur_pc(other.cur_pc)
 	, scenario(other.scenario)
 	, party(other.party)
-	, stored_pcs(other.stored_pcs)
 	, town(*this)
 	, out(*this)
 	, file(other.file)
@@ -920,11 +919,12 @@ cUniverse::cUniverse(const cUniverse& other)
 	, ghost_mode(other.ghost_mode)
 	, node_step_through(other.node_step_through)
 {
-	for(auto& p : stored_pcs) {
-		p.second = new cPlayer(*p.second);
-	}
 	town.copy(other.town);
 	out.copy(other.out);
+	for(const auto& pc : other.stored_pcs) {
+		std::unique_ptr<cPlayer> clone(new cPlayer(no_party, *pc.second));
+		stored_pcs.emplace(pc.first, std::move(clone));
+	}
 }
 
 cUniverse::cUniverse(cUniverse&& other) : town(*this), out(*this) {
@@ -1305,13 +1305,7 @@ short cUniverse::difficulty_adjust() const {
 	return adj;
 }
 
-cUniverse::~cUniverse() {
-	clear_stored_pcs();
-}
-
 void cUniverse::clear_stored_pcs() {
-	for(auto& p : stored_pcs)
-		delete p.second;
 	stored_pcs.clear();
 }
 

src/universe/universe.hpp

@@ -198,7 +198,7 @@ public:
 	
 	cScenario scenario;
 	cParty party;
-	std::map<long,cPlayer*> stored_pcs;
+	std::map<long, std::unique_ptr<cPlayer>> stored_pcs;
 	cCurTown town;
 	cCurOut out;
 	fs::path file;
@@ -214,7 +214,6 @@ public:
 	void generate_job_bank(int which, job_bank_t& bank);
 	short difficulty_adjust() const;
 	explicit cUniverse(ePartyPreset party_type = PARTY_DEFAULT);
-	~cUniverse();
 	// Copy-and-swap
 	void swap(cUniverse& other);
 	cUniverse(const cUniverse& other);