Clean up some packet-related code

src/client/client.cpp

@@ -398,11 +398,7 @@ void Client::step(float dtime)
 	if (dtime > DTIME_LIMIT)
 		dtime = DTIME_LIMIT;
 
-	m_animation_time += dtime;
-	if(m_animation_time > 60.0)
-		m_animation_time -= 60.0;
-
-	m_time_of_day_update_timer += dtime;
+	m_animation_time = fmodf(m_animation_time + dtime, 60.0f);
 
 	ReceiveAll();
 
@@ -873,14 +869,6 @@ void Client::deletingPeer(con::IPeer *peer, bool timeout)
 		m_access_denied_reason = gettext("Connection aborted (protocol error?).");
 }
 
-/*
-	u16 command
-	u16 number of files requested
-	for each file {
-		u16 length of name
-		string name
-	}
-*/
 void Client::request_media(const std::vector<std::string> &file_requests)
 {
 	std::ostringstream os(std::ios_base::binary);

src/client/client.h

@@ -538,11 +538,6 @@ private:
 	// Pending downloads of dynamic media (key: token)
 	std::vector<std::pair<u32, std::shared_ptr<SingleMediaDownloader>>> m_pending_media_downloads;
 
-	// time_of_day speed approximation for old protocol
-	bool m_time_of_day_set = false;
-	float m_last_time_of_day_f = -1.0f;
-	float m_time_of_day_update_timer = 0.0f;
-
 	// An interval for generally sending object positions and stuff
 	float m_recommended_send_interval = 0.1f;
 

src/mapnode.cpp

@@ -561,7 +561,7 @@ void MapNode::serialize(u8 *dest, u8 version) const
 	writeU8(dest+2, param1);
 	writeU8(dest+3, param2);
 }
-void MapNode::deSerialize(u8 *source, u8 version)
+void MapNode::deSerialize(const u8 *source, u8 version)
 {
 	if (!ser_ver_supported_read(version))
 		throw VersionMismatchException("ERROR: MapNode format not supported");

src/mapnode.h

@@ -296,7 +296,7 @@ struct alignas(u32) MapNode
 
 	static u32 serializedLength(u8 version);
 	void serialize(u8 *dest, u8 version) const;
-	void deSerialize(u8 *source, u8 version);
+	void deSerialize(const u8 *source, u8 version);
 
 	// Serializes or deserializes a list of nodes in bulk format (first the
 	// content of all nodes, then the param1 of all nodes, then the param2

src/network/clientpackethandler.cpp

@@ -132,17 +132,10 @@ void Client::handleCommand_AuthAccept(NetworkPacket* pkt)
 {
 	deleteAuthData();
 
-	v3f playerpos;
-	*pkt >> playerpos >> m_map_seed >> m_recommended_send_interval
+	v3f unused;
+	*pkt >> unused >> m_map_seed >> m_recommended_send_interval
 		>> m_sudo_auth_methods;
 
-	playerpos -= v3f(0, BS / 2, 0);
-
-	// Set player position
-	LocalPlayer *player = m_env.getLocalPlayer();
-	assert(player != NULL);
-	player->setPosition(playerpos);
-
 	infostream << "Client: received map seed: " << m_map_seed << std::endl;
 	infostream << "Client: received recommended send interval "
 					<< m_recommended_send_interval<<std::endl;
@@ -176,6 +169,7 @@ void Client::handleCommand_AcceptSudoMode(NetworkPacket* pkt)
 	// reset again
 	m_chosen_auth_mech = AUTH_MECHANISM_NONE;
 }
+
 void Client::handleCommand_DenySudoMode(NetworkPacket* pkt)
 {
 	ChatMessage *chatMessage = new ChatMessage(CHATMESSAGE_TYPE_SYSTEM,
@@ -193,8 +187,8 @@ void Client::handleCommand_AccessDenied(NetworkPacket* pkt)
 	m_access_denied = true;
 
 	if (pkt->getCommand() != TOCLIENT_ACCESS_DENIED) {
-		// Legacy code from 0.4.12 and older but is still used
-		// in some places of the server code
+		// Servers older than 5.6 still send TOCLIENT_ACCESS_DENIED_LEGACY sometimes.
+		// see commit a65f6f07f3a5601207b790edcc8cc945133112f7
 		if (pkt->getSize() >= 2) {
 			std::wstring wide_reason;
 			*pkt >> wide_reason;
@@ -231,9 +225,6 @@ void Client::handleCommand_AccessDenied(NetworkPacket* pkt)
 
 void Client::handleCommand_RemoveNode(NetworkPacket* pkt)
 {
-	if (pkt->getSize() < 6)
-		return;
-
 	v3s16 p;
 	*pkt >> p;
 	removeNode(p);
@@ -241,20 +232,17 @@ void Client::handleCommand_RemoveNode(NetworkPacket* pkt)
 
 void Client::handleCommand_AddNode(NetworkPacket* pkt)
 {
-	if (pkt->getSize() < 6 + MapNode::serializedLength(m_server_ser_ver))
-		return;
-
 	v3s16 p;
 	*pkt >> p;
 
-	MapNode n;
-	n.deSerialize(pkt->getU8Ptr(6), m_server_ser_ver);
+	auto *ptr = reinterpret_cast<const u8*>(pkt->getRemainingString());
+	pkt->skip(MapNode::serializedLength(m_server_ser_ver)); // performs length check
 
-	bool remove_metadata = true;
-	u32 index = 6 + MapNode::serializedLength(m_server_ser_ver);
-	if ((pkt->getSize() >= index + 1) && pkt->getU8(index)) {
-		remove_metadata = false;
-	}
+	MapNode n;
+	n.deSerialize(ptr, m_server_ser_ver);
+
+	bool remove_metadata;
+	*pkt >> remove_metadata;
 
 	addNode(p, n, remove_metadata);
 }
@@ -272,7 +260,7 @@ void Client::handleCommand_NodemetaChanged(NetworkPacket *pkt)
 	meta_updates_list.deSerialize(sstr, m_itemdef, true);
 
 	Map &map = m_env.getMap();
-	for (NodeMetadataMap::const_iterator i = meta_updates_list.begin();
+	for (auto i = meta_updates_list.begin();
 			i != meta_updates_list.end(); ++i) {
 		v3s16 pos = i->first;
 
@@ -294,7 +282,7 @@ void Client::handleCommand_BlockData(NetworkPacket* pkt)
 	v3s16 p;
 	*pkt >> p;
 
-	std::string datastring(pkt->getString(6), pkt->getSize() - 6);
+	std::string datastring(pkt->getRemainingString(), pkt->getRemainingBytes());
 	std::istringstream istr(datastring, std::ios_base::binary);
 
 	MapSector *sector;
@@ -358,46 +346,15 @@ void Client::handleCommand_TimeOfDay(NetworkPacket* pkt)
 		return;
 
 	u16 time_of_day;
-
 	*pkt >> time_of_day;
-
 	time_of_day      = time_of_day % 24000;
-	float time_speed = 0;
 
-	if (pkt->getSize() >= 2 + 4) {
-		*pkt >> time_speed;
-	}
-	else {
-		// Old message; try to approximate speed of time by ourselves
-		float time_of_day_f = (float)time_of_day / 24000.0f;
-		float tod_diff_f = 0;
-
-		if (time_of_day_f < 0.2 && m_last_time_of_day_f > 0.8)
-			tod_diff_f = time_of_day_f - m_last_time_of_day_f + 1.0f;
-		else
-			tod_diff_f = time_of_day_f - m_last_time_of_day_f;
-
-		m_last_time_of_day_f       = time_of_day_f;
-		float time_diff            = m_time_of_day_update_timer;
-		m_time_of_day_update_timer = 0;
-
-		if (m_time_of_day_set) {
-			time_speed = (3600.0f * 24.0f) * tod_diff_f / time_diff;
-			infostream << "Client: Measured time_of_day speed (old format): "
-					<< time_speed << " tod_diff_f=" << tod_diff_f
-					<< " time_diff=" << time_diff << std::endl;
-		}
-	}
+	float time_speed;
+	*pkt >> time_speed;
 
 	// Update environment
 	m_env.setTimeOfDay(time_of_day);
 	m_env.setTimeOfDaySpeed(time_speed);
-	m_time_of_day_set = true;
-
-	//u32 dr = m_env.getDayNightRatio();
-	//infostream << "Client: time_of_day=" << time_of_day
-	//		<< " time_speed=" << time_speed
-	//		<< " dr=" << dr << std::endl;
 }
 
 void Client::handleCommand_ChatMessage(NetworkPacket *pkt)
@@ -851,6 +808,8 @@ void Client::handleCommand_PlaySound(NetworkPacket* pkt)
 			pos = cao->getPosition() * (1.0f/BS);
 			vel = cao->getVelocity() * (1.0f/BS);
 		}
+		// Note that the server sends 'pos' correctly even for attached sounds,
+		// so this fallback path is not a mistake.
 		m_sound->playSoundAt(client_id, spec, pos, vel);
 		break;
 	}
@@ -883,7 +842,7 @@ void Client::handleCommand_StopSound(NetworkPacket* pkt)
 
 	*pkt >> server_id;
 
-	std::unordered_map<s32, int>::iterator i = m_sounds_server_to_client.find(server_id);
+	auto i = m_sounds_server_to_client.find(server_id);
 	if (i != m_sounds_server_to_client.end()) {
 		int client_id = i->second;
 		m_sound->stopSound(client_id);
@@ -898,9 +857,7 @@ void Client::handleCommand_FadeSound(NetworkPacket *pkt)
 
 	*pkt >> sound_id >> step >> gain;
 
-	std::unordered_map<s32, int>::const_iterator i =
-			m_sounds_server_to_client.find(sound_id);
-
+	auto i = m_sounds_server_to_client.find(sound_id);
 	if (i != m_sounds_server_to_client.end())
 		m_sound->fadeSound(i->second, step, gain);
 }
@@ -958,8 +915,8 @@ void Client::handleCommand_DetachedInventory(NetworkPacket* pkt)
 		inv = inv_it->second;
 	}
 
-	u16 ignore;
-	*pkt >> ignore; // this used to be the length of the following string, ignore it
+	// this used to be the length of the following string, ignore it
+	pkt->skip(2);
 
 	std::string contents(pkt->getRemainingString(), pkt->getRemainingBytes());
 	std::istringstream is(contents, std::ios::binary);
@@ -1009,7 +966,7 @@ void Client::handleCommand_AddParticleSpawner(NetworkPacket* pkt)
 	p.amount             = readU16(is);
 	p.time               = readF32(is);
 	if (p.time < 0)
-		throw SerializationError("particle spawner time < 0");
+		throw PacketError("particle spawner time < 0");
 
 	bool missing_end_values = false;
 	if (m_proto_ver >= 42) {
@@ -1324,10 +1281,7 @@ void Client::handleCommand_HudSetSky(NetworkPacket* pkt)
 		for (size_t i = 0; i < count; i++)
 			skybox.textures.emplace_back(deSerializeString16(is));
 
-		skybox.clouds = true;
-		try {
-			skybox.clouds = readU8(is);
-		} catch (...) {}
+		skybox.clouds = readU8(is) != 0;
 
 		// Use default skybox settings:
 		SunParams sun = SkyboxDefaults::getSunDefaults();

src/network/networkpacket.cpp

@@ -49,7 +49,13 @@ const char* NetworkPacket::getString(u32 from_offset) const
 {
 	checkReadOffset(from_offset, 0);
 
-	return (char*)&m_data[from_offset];
+	return reinterpret_cast<const char*>(&m_data[from_offset]);
+}
+
+void NetworkPacket::skip(u32 count)
+{
+	checkReadOffset(m_read_offset, count);
+	m_read_offset += count;
 }
 
 void NetworkPacket::putRawString(const char* src, u32 len)
@@ -311,24 +317,6 @@ NetworkPacket& NetworkPacket::operator>>(u8& dst)
 	return *this;
 }
 
-u8 NetworkPacket::getU8(u32 offset)
-{
-	checkReadOffset(offset, 1);
-
-	return readU8(&m_data[offset]);
-}
-
-u8* NetworkPacket::getU8Ptr(u32 from_offset)
-{
-	if (m_datasize == 0) {
-		return NULL;
-	}
-
-	checkReadOffset(from_offset, 1);
-
-	return &m_data[from_offset];
-}
-
 NetworkPacket& NetworkPacket::operator>>(u16& dst)
 {
 	checkReadOffset(m_read_offset, 2);
@@ -339,13 +327,6 @@ NetworkPacket& NetworkPacket::operator>>(u16& dst)
 	return *this;
 }
 
-u16 NetworkPacket::getU16(u32 from_offset)
-{
-	checkReadOffset(from_offset, 2);
-
-	return readU16(&m_data[from_offset]);
-}
-
 NetworkPacket& NetworkPacket::operator>>(u32& dst)
 {
 	checkReadOffset(m_read_offset, 4);

src/network/networkpacket.h

@@ -35,12 +35,16 @@ public:
 	session_t getPeerId() const { return m_peer_id; }
 	u16 getCommand() const { return m_command; }
 	u32 getRemainingBytes() const { return m_datasize - m_read_offset; }
-	const char *getRemainingString() { return getString(m_read_offset); }
 
-	// Returns a c-string without copying.
+	// Returns a pointer to buffer data.
 	// A better name for this would be getRawString()
 	const char *getString(u32 from_offset) const;
-	// major difference to putCString(): doesn't write len into the buffer
+	const char *getRemainingString() const { return getString(m_read_offset); }
+
+	// Perform length check and skip ahead by `count` bytes.
+	void skip(u32 count);
+
+	// Appends bytes from string buffer to packet
 	void putRawString(const char *src, u32 len);
 	void putRawString(std::string_view src)
 	{
@@ -63,14 +67,9 @@ public:
 	NetworkPacket &operator>>(bool &dst);
 	NetworkPacket &operator<<(bool src);
 
-	u8 getU8(u32 offset);
-
 	NetworkPacket &operator>>(u8 &dst);
 	NetworkPacket &operator<<(u8 src);
 
-	u8 *getU8Ptr(u32 offset);
-
-	u16 getU16(u32 from_offset);
 	NetworkPacket &operator>>(u16 &dst);
 	NetworkPacket &operator<<(u16 src);
 
@@ -114,6 +113,7 @@ public:
 private:
 	void checkReadOffset(u32 from_offset, u32 field_size) const;
 
+	// resize data buffer for writing
 	inline void checkDataSize(u32 field_size)
 	{
 		if (m_read_offset + field_size > m_datasize) {
@@ -124,7 +124,7 @@ private:
 
 	std::vector<u8> m_data;
 	u32 m_datasize = 0;
-	u32 m_read_offset = 0;
+	u32 m_read_offset = 0; // read and write offset
 	u16 m_command = 0;
 	session_t m_peer_id = 0;
 };

src/network/serverpackethandler.cpp

@@ -325,13 +325,6 @@ void Server::handleCommand_Init2(NetworkPacket* pkt)
 	SendTimeOfDay(peer_id, time, time_speed);
 
 	SendCSMRestrictionFlags(peer_id);
-
-	// Warnings about protocol version can be issued here
-	if (client->net_proto_version < LATEST_PROTOCOL_VERSION) {
-		SendChatMessage(peer_id, ChatMessage(CHATMESSAGE_TYPE_SYSTEM,
-			L"# Server: WARNING: YOUR CLIENT'S VERSION MAY NOT BE FULLY COMPATIBLE "
-			L"WITH THIS SERVER!"));
-	}
 }
 
 void Server::handleCommand_RequestMedia(NetworkPacket* pkt)
@@ -421,11 +414,6 @@ void Server::handleCommand_GotBlocks(NetworkPacket* pkt)
 	u8 count;
 	*pkt >> count;
 
-	if ((s16)pkt->getSize() < 1 + (int)count * 6) {
-		throw con::InvalidIncomingDataException
-				("GOTBLOCKS length is too short");
-	}
-
 	ClientInterface::AutoLock lock(m_clients);
 	RemoteClient *client = m_clients.lockedGetClientNoEx(pkt->getPeerId());
 
@@ -511,20 +499,14 @@ void Server::handleCommand_PlayerPos(NetworkPacket* pkt)
 {
 	session_t peer_id = pkt->getPeerId();
 	RemotePlayer *player = m_env->getPlayer(peer_id);
-	if (player == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!player) {
+		warningstream << FUNCTION_NAME << ": player is null" << std::endl;
 		return;
 	}
 
 	PlayerSAO *playersao = player->getPlayerSAO();
-	if (playersao == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player object for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!playersao) {
+		warningstream << FUNCTION_NAME << ": player SAO is null" << std::endl;
 		return;
 	}
 
@@ -554,12 +536,8 @@ void Server::handleCommand_DeletedBlocks(NetworkPacket* pkt)
 	u8 count;
 	*pkt >> count;
 
-	RemoteClient *client = getClient(pkt->getPeerId());
-
-	if ((s16)pkt->getSize() < 1 + (int)count * 6) {
-		throw con::InvalidIncomingDataException
-				("DELETEDBLOCKS length is too short");
-	}
+	ClientInterface::AutoLock lock(m_clients);
+	RemoteClient *client = m_clients.lockedGetClientNoEx(pkt->getPeerId());
 
 	for (u16 i = 0; i < count; i++) {
 		v3s16 p;
@@ -572,28 +550,19 @@ void Server::handleCommand_InventoryAction(NetworkPacket* pkt)
 {
 	session_t peer_id = pkt->getPeerId();
 	RemotePlayer *player = m_env->getPlayer(peer_id);
-
-	if (player == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!player) {
+		warningstream << FUNCTION_NAME << ": player is null" << std::endl;
 		return;
 	}
 
 	PlayerSAO *playersao = player->getPlayerSAO();
-	if (playersao == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player object for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!playersao) {
+		warningstream << FUNCTION_NAME << ": player SAO is null" << std::endl;
 		return;
 	}
 
 	// Strip command and create a stream
 	std::string datastring(pkt->getString(0), pkt->getSize());
-	verbosestream << "TOSERVER_INVENTORY_ACTION: data=" << datastring
-		<< std::endl;
 	std::istringstream is(datastring, std::ios_base::binary);
 	// Create an action
 	std::unique_ptr<InventoryAction> a(InventoryAction::deSerialize(is));
@@ -777,15 +746,12 @@ void Server::handleCommand_ChatMessage(NetworkPacket* pkt)
 
 	session_t peer_id = pkt->getPeerId();
 	RemotePlayer *player = m_env->getPlayer(peer_id);
-	if (player == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!player) {
+		warningstream << FUNCTION_NAME << ": player is null" << std::endl;
 		return;
 	}
 
-	std::string name = player->getName();
+	const auto &name = player->getName();
 
 	std::wstring answer_to_sender = handleChat(name, message, true, player);
 	if (!answer_to_sender.empty()) {
@@ -803,29 +769,22 @@ void Server::handleCommand_Damage(NetworkPacket* pkt)
 
 	session_t peer_id = pkt->getPeerId();
 	RemotePlayer *player = m_env->getPlayer(peer_id);
-
-	if (player == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!player) {
+		warningstream << FUNCTION_NAME << ": player is null" << std::endl;
 		return;
 	}
 
 	PlayerSAO *playersao = player->getPlayerSAO();
-	if (playersao == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player object for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!playersao) {
+		warningstream << FUNCTION_NAME << ": player SAO is null" << std::endl;
 		return;
 	}
 
 	if (!playersao->isImmortal()) {
 		if (playersao->isDead()) {
-			verbosestream << "Server::ProcessData(): Info: "
+			verbosestream << "Server: "
 				"Ignoring damage as player " << player->getName()
-				<< " is already dead." << std::endl;
+				<< " is already dead" << std::endl;
 			return;
 		}
 
@@ -845,21 +804,14 @@ void Server::handleCommand_PlayerItem(NetworkPacket* pkt)
 
 	session_t peer_id = pkt->getPeerId();
 	RemotePlayer *player = m_env->getPlayer(peer_id);
-
-	if (player == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!player) {
+		warningstream << FUNCTION_NAME << ": player is null" << std::endl;
 		return;
 	}
 
 	PlayerSAO *playersao = player->getPlayerSAO();
-	if (playersao == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player object for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!playersao) {
+		warningstream << FUNCTION_NAME << ": player SAO is null" << std::endl;
 		return;
 	}
 
@@ -868,7 +820,7 @@ void Server::handleCommand_PlayerItem(NetworkPacket* pkt)
 	*pkt >> item;
 
 	if (item >= player->getMaxHotbarItemcount()) {
-		actionstream << "Player: " << player->getName()
+		actionstream << "Player " << player->getName()
 			<< " tried to access item=" << item
 			<< " out of hotbar_itemcount="
 			<< player->getMaxHotbarItemcount()
@@ -933,21 +885,14 @@ void Server::handleCommand_Interact(NetworkPacket *pkt)
 
 	session_t peer_id = pkt->getPeerId();
 	RemotePlayer *player = m_env->getPlayer(peer_id);
-
-	if (player == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!player) {
+		warningstream << FUNCTION_NAME << ": player is null" << std::endl;
 		return;
 	}
 
 	PlayerSAO *playersao = player->getPlayerSAO();
-	if (playersao == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player object for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!playersao) {
+		warningstream << FUNCTION_NAME << ": player SAO is null" << std::endl;
 		return;
 	}
 
@@ -972,7 +917,7 @@ void Server::handleCommand_Interact(NetworkPacket *pkt)
 	// Update wielded item
 
 	if (item_i >= player->getMaxHotbarItemcount()) {
-		actionstream << "Player: " << player->getName()
+		actionstream << "Player " << player->getName()
 			<< " tried to access item=" << item_i
 			<< " out of hotbar_itemcount="
 			<< player->getMaxHotbarItemcount()
@@ -1363,21 +1308,14 @@ void Server::handleCommand_NodeMetaFields(NetworkPacket* pkt)
 {
 	session_t peer_id = pkt->getPeerId();
 	RemotePlayer *player = m_env->getPlayer(peer_id);
-
-	if (player == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!player) {
+		warningstream << FUNCTION_NAME << ": player is null" << std::endl;
 		return;
 	}
 
 	PlayerSAO *playersao = player->getPlayerSAO();
-	if (playersao == NULL) {
-		errorstream <<
-			"Server::ProcessData(): Canceling: No player object for peer_id=" <<
-			peer_id << " disconnecting peer!" << std::endl;
-		DisconnectPeer(peer_id);
+	if (!playersao) {
+		warningstream << FUNCTION_NAME << ": player SAO is null" << std::endl;
 		return;
 	}
 
@@ -1450,25 +1388,26 @@ void Server::handleCommand_InventoryFields(NetworkPacket* pkt)
 	}
 
 	// verify that we displayed the formspec to the user
-	const auto peer_state_iterator = m_formspec_state_data.find(peer_id);
-	if (peer_state_iterator != m_formspec_state_data.end()) {
-		const std::string &server_formspec_name = peer_state_iterator->second;
+	const auto it = m_formspec_state_data.find(peer_id);
+	if (it != m_formspec_state_data.end()) {
+		const auto &server_formspec_name = it->second;
 		if (client_formspec_name == server_formspec_name) {
-			auto it = fields.find("quit");
-			if (it != fields.end() && it->second == "true")
-				m_formspec_state_data.erase(peer_state_iterator);
+			// delete state if formspec was closed
+			auto it2 = fields.find("quit");
+			if (it2 != fields.end() && it2->second == "true")
+				m_formspec_state_data.erase(it);
 
 			m_script->on_playerReceiveFields(playersao, client_formspec_name, fields);
 			return;
 		}
-		actionstream << "'" << player->getName()
-			<< "' submitted formspec ('" << client_formspec_name
+		actionstream << player->getName()
+			<< " submitted formspec ('" << client_formspec_name
 			<< "') but the name of the formspec doesn't match the"
 			" expected name ('" << server_formspec_name << "')";
 
 	} else {
-		actionstream << "'" << player->getName()
-			<< "' submitted formspec ('" << client_formspec_name
+		actionstream << player->getName()
+			<< " submitted formspec ('" << client_formspec_name
 			<< "') but server hasn't sent formspec to client";
 	}
 	actionstream << ", possible exploitation attempt" << std::endl;

src/server.cpp

@@ -1559,10 +1559,6 @@ void Server::SendChatMessage(session_t peer_id, const ChatMessage &message)
 		<< static_cast<u64>(message.timestamp);
 
 	if (peer_id != PEER_ID_INEXISTENT) {
-		RemotePlayer *player = m_env->getPlayer(peer_id);
-		if (!player)
-			return;
-
 		Send(&pkt);
 	} else {
 		m_clients.sendToAll(&pkt);
@@ -2921,7 +2917,7 @@ void Server::acceptAuth(session_t peer_id, bool forSudoMode)
 
 		NetworkPacket resp_pkt(TOCLIENT_AUTH_ACCEPT, 1 + 6 + 8 + 4, peer_id);
 
-		resp_pkt << v3f(0,0,0) << (u64) m_env->getServerMap().getSeed()
+		resp_pkt << v3f() << (u64) m_env->getServerMap().getSeed()
 				<< g_settings->getFloat("dedicated_server_step")
 				<< client->allowed_auth_mechs;
 

src/unittest/test_connection.cpp

@@ -273,7 +273,7 @@ void TestConnection::testConnectSendReceive()
 		UASSERT(server.ReceiveTimeoutMs(&recvpacket, timeout_ms));
 		infostream << "** Server received: peer_id=" << pkt.getPeerId()
 				<< ", size=" << pkt.getSize()
-				<< ", data=" << (const char*)pkt.getU8Ptr(0)
+				<< ", data=" << pkt.getString(0)
 				<< std::endl;
 
 		auto recvdata = pkt.oldForgePacket();
@@ -298,7 +298,7 @@ void TestConnection::testConnectSendReceive()
 				infostream << " ";
 			char buf[10];
 			porting::mt_snprintf(buf, sizeof(buf), "%.2X",
-				((int)((const char *)pkt.getU8Ptr(0))[i]) & 0xff);
+				((int)(pkt.getString(0))[i]) & 0xff);
 			infostream<<buf;
 		}
 		if (datasize > 20)

src/util/serialize.cpp

@@ -24,6 +24,7 @@ std::string serializeString16(std::string_view plain)
 	std::string s;
 	char buf[2];
 
+	static_assert(STRING_MAX_LEN <= U16_MAX);
 	if (plain.size() > STRING_MAX_LEN)
 		throw SerializationError("String too long for serializeString16");
 	s.reserve(2 + plain.size());
@@ -51,7 +52,7 @@ std::string deSerializeString16(std::istream &is)
 	s.resize(s_size);
 	is.read(&s[0], s_size);
 	if (is.gcount() != s_size)
-		throw SerializationError("deSerializeString16: couldn't read all chars");
+		throw SerializationError("deSerializeString16: truncated");
 
 	return s;
 }
@@ -66,6 +67,7 @@ std::string serializeString32(std::string_view plain)
 	std::string s;
 	char buf[4];
 
+	static_assert(LONG_STRING_MAX_LEN <= U32_MAX);
 	if (plain.size() > LONG_STRING_MAX_LEN)
 		throw SerializationError("String too long for serializeLongString");
 	s.reserve(4 + plain.size());
@@ -98,7 +100,7 @@ std::string deSerializeString32(std::istream &is)
 	s.resize(s_size);
 	is.read(&s[0], s_size);
 	if ((u32)is.gcount() != s_size)
-		throw SerializationError("deSerializeLongString: couldn't read all chars");
+		throw SerializationError("deSerializeLongString: truncated");
 
 	return s;
 }