Network: Fix serialization version checks (#15477)

This fixes some incorrect assumptions that the read and write version ranges are identical - whereas they're in fact not.

src/client/client.cpp

@@ -1128,7 +1128,7 @@ void Client::sendInit(const std::string &playerName)
 {
 	NetworkPacket pkt(TOSERVER_INIT, 1 + 2 + 2 + (1 + playerName.size()));
 
-	pkt << (u8) SER_FMT_VER_HIGHEST_READ << (u16) 0;
+	pkt << SER_FMT_VER_HIGHEST_READ << (u16) 0 /* unused */;
 	pkt << CLIENT_PROTOCOL_VERSION_MIN << LATEST_PROTOCOL_VERSION;
 	pkt << playerName;
 

src/main.cpp

@@ -24,6 +24,7 @@
 #include "config.h"
 #include "player.h"
 #include "porting.h"
+#include "serialization.h" // SER_FMT_VER_HIGHEST_*
 #include "network/socket.h"
 #include "mapblock.h"
 #if USE_CURSES

src/mapblock.cpp

@@ -307,11 +307,9 @@ static void correctBlockNodeIds(const NameIdMapping *nimap, MapNode *nodes,
 
 void MapBlock::serialize(std::ostream &os_compressed, u8 version, bool disk, int compression_level)
 {
-	if(!ser_ver_supported(version))
+	if (!ser_ver_supported_write(version))
 		throw VersionMismatchException("ERROR: MapBlock format not supported");
 
-	FATAL_ERROR_IF(version < SER_FMT_VER_LOWEST_WRITE, "Serialization version error");
-
 	std::ostringstream os_raw(std::ios_base::binary);
 	std::ostream &os = version >= 29 ? os_raw : os_compressed;
 
@@ -423,7 +421,7 @@ void MapBlock::serializeNetworkSpecific(std::ostream &os)
 
 void MapBlock::deSerialize(std::istream &in_compressed, u8 version, bool disk)
 {
-	if(!ser_ver_supported(version))
+	if (!ser_ver_supported_read(version))
 		throw VersionMismatchException("ERROR: MapBlock format not supported");
 
 	TRACESTREAM(<<"MapBlock::deSerialize "<<getPos()<<std::endl);

src/mapnode.cpp

@@ -8,7 +8,7 @@
 #include "nodedef.h"
 #include "map.h"
 #include "content_mapnode.h" // For mapnode_translate_*_internal
-#include "serialization.h" // For ser_ver_supported
+#include "serialization.h" // For ser_ver_supported_*
 #include "util/serialize.h"
 #include "log.h"
 #include "util/directiontables.h"
@@ -620,7 +620,7 @@ s8 MapNode::addLevel(const NodeDefManager *nodemgr, s16 add)
 
 u32 MapNode::serializedLength(u8 version)
 {
-	if(!ser_ver_supported(version))
+	if (!ser_ver_supported_read(version))
 		throw VersionMismatchException("ERROR: MapNode format not supported");
 
 	if (version == 0)
@@ -636,7 +636,7 @@ u32 MapNode::serializedLength(u8 version)
 }
 void MapNode::serialize(u8 *dest, u8 version) const
 {
-	if(!ser_ver_supported(version))
+	if (!ser_ver_supported_write(version))
 		throw VersionMismatchException("ERROR: MapNode format not supported");
 
 	// Can't do this anymore; we have 16-bit dynamically allocated node IDs
@@ -651,7 +651,7 @@ void MapNode::serialize(u8 *dest, u8 version) const
 }
 void MapNode::deSerialize(u8 *source, u8 version)
 {
-	if(!ser_ver_supported(version))
+	if (!ser_ver_supported_read(version))
 		throw VersionMismatchException("ERROR: MapNode format not supported");
 
 	if(version <= 21)
@@ -679,18 +679,12 @@ Buffer<u8> MapNode::serializeBulk(int version,
 		const MapNode *nodes, u32 nodecount,
 		u8 content_width, u8 params_width)
 {
-	if (!ser_ver_supported(version))
+	if (!ser_ver_supported_write(version))
 		throw VersionMismatchException("ERROR: MapNode format not supported");
 
 	sanity_check(content_width == 2);
 	sanity_check(params_width == 2);
 
-	// Can't do this anymore; we have 16-bit dynamically allocated node IDs
-	// in memory; conversion just won't work in this direction.
-	if (version < 24)
-		throw SerializationError("MapNode::serializeBulk: serialization to "
-				"version < 24 not possible");
-
 	Buffer<u8> databuf(nodecount * (content_width + params_width));
 
 	// Writing to the buffer linearly is faster
@@ -712,13 +706,13 @@ void MapNode::deSerializeBulk(std::istream &is, int version,
 		MapNode *nodes, u32 nodecount,
 		u8 content_width, u8 params_width)
 {
-	if(!ser_ver_supported(version))
+	if (!ser_ver_supported_read(version))
 		throw VersionMismatchException("ERROR: MapNode format not supported");
 
 	if (version < 22
 			|| (content_width != 1 && content_width != 2)
 			|| params_width != 2)
-		FATAL_ERROR("Deserialize bulk node data error");
+		throw SerializationError("Deserialize bulk node data error");
 
 	// read data
 	const u32 len = nodecount * (content_width + params_width);

src/network/clientpackethandler.cpp

@@ -63,7 +63,7 @@ void Client::handleCommand_Hello(NetworkPacket* pkt)
 	if (pkt->getSize() < 1)
 		return;
 
-	u8 serialization_ver;
+	u8 serialization_ver; // negotiated value
 	u16 proto_ver;
 	u16 unused_compression_mode;
 	u32 auth_mechs;
@@ -80,9 +80,9 @@ void Client::handleCommand_Hello(NetworkPacket* pkt)
 			<< ", proto_ver=" << proto_ver
 			<< ". Doing auth with mech " << chosen_auth_mechanism << std::endl;
 
-	if (!ser_ver_supported(serialization_ver)) {
+	if (!ser_ver_supported_read(serialization_ver)) {
 		infostream << "Client: TOCLIENT_HELLO: Server sent "
-				<< "unsupported ser_fmt_ver"<< std::endl;
+				<< "unsupported ser_fmt_ver=" << (int)serialization_ver << std::endl;
 		return;
 	}
 

src/network/serverpackethandler.cpp

@@ -12,6 +12,7 @@
 #include "remoteplayer.h"
 #include "rollback_interface.h"
 #include "scripting_server.h"
+#include "serialization.h"
 #include "settings.h"
 #include "tool.h"
 #include "version.h"
@@ -83,34 +84,27 @@ void Server::handleCommand_Init(NetworkPacket* pkt)
 	if (denyIfBanned(peer_id))
 		return;
 
-	// First byte after command is maximum supported
-	// serialization version
-	u8 client_max;
+	u8 max_ser_ver; // SER_FMT_VER_HIGHEST_READ (of client)
 	u16 unused;
-	u16 min_net_proto_version = 0;
+	u16 min_net_proto_version;
 	u16 max_net_proto_version;
 	std::string playerName;
 
-	*pkt >> client_max >> unused >> min_net_proto_version
-			>> max_net_proto_version >> playerName;
+	*pkt >> max_ser_ver >> unused
+			>> min_net_proto_version >> max_net_proto_version
+			>> playerName;
 
-	u8 our_max = SER_FMT_VER_HIGHEST_READ;
 	// Use the highest version supported by both
-	u8 depl_serial_v = std::min(client_max, our_max);
-	// If it's lower than the lowest supported, give up.
-#if SER_FMT_VER_LOWEST_READ > 0
-	if (depl_serial_v < SER_FMT_VER_LOWEST_READ)
-		depl_serial_v = SER_FMT_VER_INVALID;
-#endif
+	const u8 serialization_ver = std::min(max_ser_ver, SER_FMT_VER_HIGHEST_WRITE);
 
-	if (depl_serial_v == SER_FMT_VER_INVALID) {
+	if (!ser_ver_supported_write(serialization_ver)) {
 		actionstream << "Server: A mismatched client tried to connect from " <<
-			addr_s << " ser_fmt_max=" << (int)client_max << std::endl;
+			addr_s << " ser_fmt_max=" << (int)serialization_ver << std::endl;
 		DenyAccess(peer_id, SERVER_ACCESSDENIED_WRONG_VERSION);
 		return;
 	}
 
-	client->setPendingSerializationVersion(depl_serial_v);
+	client->setPendingSerializationVersion(serialization_ver);
 
 	/*
 		Read and check network protocol version
@@ -263,8 +257,9 @@ void Server::handleCommand_Init(NetworkPacket* pkt)
 
 	NetworkPacket resp_pkt(TOCLIENT_HELLO, 0, peer_id);
 
-	resp_pkt << depl_serial_v << u16(0) << net_proto_version
-		<< auth_mechs << std::string_view();
+	resp_pkt << serialization_ver << u16(0) /* unused */
+		<< net_proto_version
+		<< auth_mechs << std::string_view() /* unused */;
 
 	Send(&resp_pkt);
 

src/serialization.h

@@ -50,23 +50,30 @@
 	28: Added "private" flag to NodeMetadata
 	29: Switched compression to zstd, a bit of reorganization
 */
+
 // This represents an uninitialized or invalid format
-#define SER_FMT_VER_INVALID 255
+constexpr u8 SER_FMT_VER_INVALID = 255;
 // Highest supported serialization version
-#define SER_FMT_VER_HIGHEST_READ 29
+constexpr u8 SER_FMT_VER_HIGHEST_READ = 29;
 // Saved on disk version
-#define SER_FMT_VER_HIGHEST_WRITE 29
+constexpr u8 SER_FMT_VER_HIGHEST_WRITE = 29;
 // Lowest supported serialization version
-#define SER_FMT_VER_LOWEST_READ 0
+constexpr u8 SER_FMT_VER_LOWEST_READ = 0;
 // Lowest serialization version for writing
 // Can't do < 24 anymore; we have 16-bit dynamically allocated node IDs
 // in memory; conversion just won't work in this direction.
-#define SER_FMT_VER_LOWEST_WRITE 24
+constexpr u8 SER_FMT_VER_LOWEST_WRITE = 24;
 
-inline bool ser_ver_supported(s32 v) {
+inline bool ser_ver_supported_read(s32 v)
+{
 	return v >= SER_FMT_VER_LOWEST_READ && v <= SER_FMT_VER_HIGHEST_READ;
 }
 
+inline bool ser_ver_supported_write(s32 v)
+{
+	return v >= SER_FMT_VER_LOWEST_WRITE && v <= SER_FMT_VER_HIGHEST_WRITE;
+}
+
 /*
 	Compression functions
 */

src/server.cpp

@@ -21,6 +21,7 @@
 #include "filesys.h"
 #include "mapblock.h"
 #include "server/serveractiveobject.h"
+#include "serialization.h" // SER_FMT_VER_INVALID
 #include "settings.h"
 #include "profiler.h"
 #include "log.h"

src/server.h

@@ -8,7 +8,6 @@
 #include "map.h"
 #include "hud.h"
 #include "gamedef.h"
-#include "serialization.h" // For SER_FMT_VER_INVALID
 #include "content/mods.h"
 #include "inventorymanager.h"
 #include "content/subgames.h"

src/server/clientiface.cpp

@@ -8,6 +8,7 @@
 #include "network/connection.h"
 #include "network/serveropcodes.h"
 #include "remoteplayer.h"
+#include "serialization.h" // SER_FMT_VER_INVALID
 #include "settings.h"
 #include "mapblock.h"
 #include "serverenvironment.h"
@@ -51,6 +52,8 @@ std::string ClientInterface::state2Name(ClientState state)
 }
 
 RemoteClient::RemoteClient() :
+	serialization_version(SER_FMT_VER_INVALID),
+	m_pending_serialization_version(SER_FMT_VER_INVALID),
 	m_max_simul_sends(g_settings->getU16("max_simultaneous_block_sends_per_client")),
 	m_min_time_from_building(
 		g_settings->getFloat("full_block_send_enable_min_time_from_building")),

src/server/clientiface.h

@@ -7,7 +7,6 @@
 #include "irr_v3d.h"                   // for irrlicht datatypes
 
 #include "constants.h"
-#include "serialization.h"             // for SER_FMT_VER_INVALID
 #include "network/networkpacket.h"
 #include "network/networkprotocol.h"
 #include "network/address.h"
@@ -219,7 +218,7 @@ public:
 	//       Also, the client must be moved to some other container.
 	session_t peer_id = PEER_ID_INEXISTENT;
 	// The serialization version to use with the client
-	u8 serialization_version = SER_FMT_VER_INVALID;
+	u8 serialization_version;
 	//
 	u16 net_proto_version = 0;
 
@@ -333,7 +332,7 @@ public:
 
 private:
 	// Version is stored in here after INIT before INIT2
-	u8 m_pending_serialization_version = SER_FMT_VER_INVALID;
+	u8 m_pending_serialization_version;
 
 	/* current state of client */
 	ClientState m_state = CS_Created;