RemotePlayer: make peer ID always reflect the validity of PlayerSAO

Upon disconnect, RemotePlayer still had a peer ID assigned even though
the PlayerSAO object was maked as gone (for removal). This commit makes
that the following always holds true:

	(!sao || sao->isGone()) === (peer_id == PEER_ID_INEXISTENT)

src/network/serverpackethandler.cpp

@@ -512,7 +512,7 @@ void Server::process_PlayerPos(RemotePlayer *player, PlayerSAO *playersao,
 	if (playersao->checkMovementCheat()) {
 		// Call callbacks
 		m_script->on_cheat(playersao, "moved_too_fast");
-		SendMovePlayer(pkt->getPeerId());
+		SendMovePlayer(playersao);
 	}
 }
 

src/remoteplayer.cpp

@@ -69,6 +69,11 @@ RemotePlayer::RemotePlayer(const char *name, IItemDefManager *idef):
 	m_star_params   = SkyboxDefaults::getStarDefaults();
 }
 
+RemotePlayer::~RemotePlayer()
+{
+	if (m_sao)
+		m_sao->setPlayer(nullptr);
+}
 
 RemotePlayerChatResult RemotePlayer::canSendChatMessage()
 {

src/remoteplayer.h

@@ -42,7 +42,7 @@ class RemotePlayer : public Player
 
 public:
 	RemotePlayer(const char *name, IItemDefManager *idef);
-	virtual ~RemotePlayer() = default;
+	virtual ~RemotePlayer();
 
 	PlayerSAO *getPlayerSAO() { return m_sao; }
 	void setPlayerSAO(PlayerSAO *sao) { m_sao = sao; }
@@ -135,6 +135,7 @@ class RemotePlayer : public Player
 	u16 protocol_version = 0;
 	u16 formspec_version = 0;
 
+	/// returns PEER_ID_INEXISTENT when PlayerSAO is not ready
 	session_t getPeerId() const { return m_peer_id; }
 
 	void setPeerId(session_t peer_id) { m_peer_id = peer_id; }

src/server.cpp

@@ -1105,7 +1105,7 @@ PlayerSAO* Server::StageTwoClientInit(session_t peer_id)
 	/*
 		Send complete position information
 	*/
-	SendMovePlayer(peer_id);
+	SendMovePlayer(playersao);
 
 	// Send privileges
 	SendPlayerPrivileges(peer_id);
@@ -1900,17 +1900,12 @@ void Server::SendPlayerBreath(PlayerSAO *sao)
 	SendBreath(sao->getPeerID(), sao->getBreath());
 }
 
-void Server::SendMovePlayer(session_t peer_id)
+void Server::SendMovePlayer(PlayerSAO *sao)
 {
-	RemotePlayer *player = m_env->getPlayer(peer_id);
-	assert(player);
-	PlayerSAO *sao = player->getPlayerSAO();
-	assert(sao);
-
 	// Send attachment updates instantly to the client prior updating position
 	sao->sendOutdatedData();
 
-	NetworkPacket pkt(TOCLIENT_MOVE_PLAYER, sizeof(v3f) + sizeof(f32) * 2, peer_id);
+	NetworkPacket pkt(TOCLIENT_MOVE_PLAYER, sizeof(v3f) + sizeof(f32) * 2, sao->getPeerID());
 	pkt << sao->getBasePosition() << sao->getLookPitch() << sao->getRotation().Y;
 
 	{
@@ -3929,6 +3924,23 @@ PlayerSAO* Server::emergePlayer(const char *name, session_t peer_id, u16 proto_v
 		return NULL;
 	}
 
+	/*
+		Object construction sequence/hierarchy
+		--------------------------------------
+		1. RemoteClient (tightly connection-bound)
+		2. RemotePlayer (controls, in-game appearance)
+		3. PlayerSAO (movable object in-game)
+			PlayerSAO controls the peer_id assignment of RemotePlayer,
+			indicating whether the player is ready
+
+		Destruction sequence
+		--------------------
+		1. PlayerSAO pending removal flag
+		2. PlayerSAO save data before free
+		3. RemotePlayer, then PlayerSAO freed
+		4. RemoteClient freed
+	*/
+
 	if (!player) {
 		player = new RemotePlayer(name, idef());
 	}

src/server.h

@@ -364,7 +364,7 @@ class Server : public con::PeerHandler, public MapEventReceiver,
 	void SendPlayerHP(PlayerSAO *sao, bool effect);
 	void SendPlayerBreath(PlayerSAO *sao);
 	void SendInventory(PlayerSAO *playerSAO, bool incremental);
-	void SendMovePlayer(session_t peer_id);
+	void SendMovePlayer(PlayerSAO *sao);
 	void SendMovePlayerRel(session_t peer_id, const v3f &added_pos);
 	void SendPlayerSpeed(session_t peer_id, const v3f &added_vel);
 	void SendPlayerFov(session_t peer_id);

src/server/player_sao.cpp

@@ -29,10 +29,10 @@ PlayerSAO::PlayerSAO(ServerEnvironment *env_, RemotePlayer *player_, session_t p
 		bool is_singleplayer):
 	UnitSAO(env_, v3f(0,0,0)),
 	m_player(player_),
-	m_peer_id(peer_id_),
+	m_peer_id_initial(peer_id_),
 	m_is_singleplayer(is_singleplayer)
 {
-	SANITY_CHECK(m_peer_id != PEER_ID_INEXISTENT);
+	SANITY_CHECK(m_peer_id_initial != PEER_ID_INEXISTENT);
 
 	m_prop.hp_max = PLAYER_MAX_HP_DEFAULT;
 	m_prop.breath_max = PLAYER_MAX_BREATH_DEFAULT;
@@ -88,7 +88,7 @@ void PlayerSAO::addedToEnvironment(u32 dtime_s)
 	ServerActiveObject::addedToEnvironment(dtime_s);
 	ServerActiveObject::setBasePosition(m_base_position);
 	m_player->setPlayerSAO(this);
-	m_player->setPeerId(m_peer_id);
+	m_player->setPeerId(m_peer_id_initial);
 	m_last_good_position = m_base_position;
 }
 
@@ -236,7 +236,7 @@ void PlayerSAO::step(float dtime, bool send_recommended)
 			" is attached to nonexistent parent. This is a bug." << std::endl;
 		clearParentAttachment();
 		setBasePosition(m_last_good_position);
-		m_env->getGameDef()->SendMovePlayer(m_peer_id);
+		m_env->getGameDef()->SendMovePlayer(this);
 	}
 
 	//dstream<<"PlayerSAO::step: dtime: "<<dtime<<std::endl;
@@ -357,14 +357,14 @@ void PlayerSAO::setPos(const v3f &pos)
 
 	// Send mapblock of target location
 	v3s16 blockpos = v3s16(pos.X / MAP_BLOCKSIZE, pos.Y / MAP_BLOCKSIZE, pos.Z / MAP_BLOCKSIZE);
-	m_env->getGameDef()->SendBlock(m_peer_id, blockpos);
+	m_env->getGameDef()->SendBlock(getPeerID(), blockpos);
 
 	setBasePosition(pos);
 	// Movement caused by this command is always valid
 	m_last_good_position = getBasePosition();
 	m_move_pool.empty();
 	m_time_from_last_teleport = 0.0;
-	m_env->getGameDef()->SendMovePlayer(m_peer_id);
+	m_env->getGameDef()->SendMovePlayer(this);
 }
 
 void PlayerSAO::addPos(const v3f &added_pos)
@@ -381,14 +381,14 @@ void PlayerSAO::addPos(const v3f &added_pos)
 	// Send mapblock of target location
 	v3f pos = getBasePosition() + added_pos;
 	v3s16 blockpos = v3s16(pos.X / MAP_BLOCKSIZE, pos.Y / MAP_BLOCKSIZE, pos.Z / MAP_BLOCKSIZE);
-	m_env->getGameDef()->SendBlock(m_peer_id, blockpos);
+	m_env->getGameDef()->SendBlock(getPeerID(), blockpos);
 
 	setBasePosition(pos);
 	// Movement caused by this command is always valid
 	m_last_good_position = getBasePosition();
 	m_move_pool.empty();
 	m_time_from_last_teleport = 0.0;
-	m_env->getGameDef()->SendMovePlayerRel(m_peer_id, added_pos);
+	m_env->getGameDef()->SendMovePlayerRel(getPeerID(), added_pos);
 }
 
 void PlayerSAO::moveTo(v3f pos, bool continuous)
@@ -401,7 +401,7 @@ void PlayerSAO::moveTo(v3f pos, bool continuous)
 	m_last_good_position = getBasePosition();
 	m_move_pool.empty();
 	m_time_from_last_teleport = 0.0;
-	m_env->getGameDef()->SendMovePlayer(m_peer_id);
+	m_env->getGameDef()->SendMovePlayer(this);
 }
 
 void PlayerSAO::setPlayerYaw(const float yaw)
@@ -433,7 +433,7 @@ void PlayerSAO::setWantedRange(const s16 range)
 void PlayerSAO::setPlayerYawAndSend(const float yaw)
 {
 	setPlayerYaw(yaw);
-	m_env->getGameDef()->SendMovePlayer(m_peer_id);
+	m_env->getGameDef()->SendMovePlayer(this);
 }
 
 void PlayerSAO::setLookPitch(const float pitch)
@@ -447,7 +447,7 @@ void PlayerSAO::setLookPitch(const float pitch)
 void PlayerSAO::setLookPitchAndSend(const float pitch)
 {
 	setLookPitch(pitch);
-	m_env->getGameDef()->SendMovePlayer(m_peer_id);
+	m_env->getGameDef()->SendMovePlayer(this);
 }
 
 u32 PlayerSAO::punch(v3f dir,
@@ -578,16 +578,22 @@ bool PlayerSAO::setWieldedItem(const ItemStack &item)
 
 void PlayerSAO::disconnected()
 {
-	m_peer_id = PEER_ID_INEXISTENT;
 	markForRemoval();
+	m_player->setPeerId(PEER_ID_INEXISTENT);
+}
+
+session_t PlayerSAO::getPeerID() const
+{
+	// This function must not be called before the player
+	// is added to the environment
+	SANITY_CHECK(m_player);
+	return m_player->getPeerId();
 }
 
 void PlayerSAO::unlinkPlayerSessionAndSave()
 {
 	assert(m_player->getPlayerSAO() == this);
-	m_player->setPeerId(PEER_ID_INEXISTENT);
 	m_env->savePlayer(m_player);
-	m_player->setPlayerSAO(NULL);
 	m_env->removePlayer(m_player);
 }
 

src/server/player_sao.h

@@ -142,8 +142,9 @@ class PlayerSAO : public UnitSAO
 
 	void disconnected();
 
+	void setPlayer(RemotePlayer *player) { m_player = player; }
 	RemotePlayer *getPlayer() { return m_player; }
-	session_t getPeerID() const { return m_peer_id; }
+	session_t getPeerID() const;
 
 	// Cheat prevention
 
@@ -193,7 +194,7 @@ class PlayerSAO : public UnitSAO
 	std::string generateUpdatePhysicsOverrideCommand() const;
 
 	RemotePlayer *m_player = nullptr;
-	session_t m_peer_id = 0;
+	session_t m_peer_id_initial = 0; ///< only used to initialize RemotePlayer
 
 	// Cheat prevention
 	LagPool m_dig_pool;

src/serverenvironment.cpp

@@ -1640,11 +1640,11 @@ void ServerEnvironment::step(float dtime)
 
 	// Send outdated player inventories
 	for (RemotePlayer *player : m_players) {
-		if (player->getPeerId() == PEER_ID_INEXISTENT)
+		PlayerSAO *sao = player->getPlayerSAO();
+		if (!sao || sao->isGone())
 			continue;
 
-		PlayerSAO *sao = player->getPlayerSAO();
-		if (sao && player->inventory.checkModified())
+		if (player->inventory.checkModified())
 			m_server->SendInventory(sao, true);
 	}