Safer checks in type serializer

libretroshare/src/serialiser/rsserial.cc

@@ -125,9 +125,7 @@ RsItem::RsItem(uint8_t ver, uint8_t cls, uint8_t t, uint8_t subtype)
 	type = (ver << 24) + (cls << 16) + (t << 8) + subtype;
 }
 
-RsItem::~RsItem()
+RsItem::~RsItem() = default;
-{
-}
 
 void RsItem::print_string(std::string &out, uint16_t indent)
 {
@@ -243,10 +241,7 @@ uint32_t    RsSerialType::PacketId() const
 
 
 
-RsSerialiser::RsSerialiser()
+RsSerialiser::RsSerialiser() = default;
-{
-	return;
-}
 
 
 RsSerialiser::~RsSerialiser()
@@ -559,17 +554,7 @@ std::ostream &RsRawItem::print(std::ostream &out, uint16_t indent)
 	return out;
 }
 
-
+uint32_t getRsPktMaxSize() { return RsSerialiser::MAX_SERIAL_SIZE; }
-uint32_t getRsPktMaxSize()
-{
-	//return 65535; /* 2^16 (old artifical low size) */
-	//return 1048575; /* 2^20 -1 (Too Big! - must remove fixed static buffers first) */
-	/* Remember that every pqistreamer allocates an input buffer of this size!
-	 * So don't make it too big!
-	 */
-	return 262143; /* 2^18 -1 */
-}
-
 
 uint32_t getRsPktBaseSize()
 {

libretroshare/src/serialiser/rsserial.h

@@ -19,16 +19,16 @@
  * along with this program. If not, see <https://www.gnu.org/licenses/>.       *
  *                                                                             *
  *******************************************************************************/
-#ifndef RS_BASE_SERIALISER_H
+#pragma once
-#define RS_BASE_SERIALISER_H
 
-#include <stdlib.h>
+#include <cstring>
-#include <string.h>
 #include <map>
 #include <string>
 #include <iosfwd>
-#include <stdlib.h>
+#include <cstdlib>
-#include <stdint.h>
+#include <cstdint>
+
+#include "util/rsdeprecate.h"
 
 /*******************************************************************
  * This is the Top-Level serialiser/deserialise, 
@@ -66,7 +66,11 @@ class RsSerialType ;
 
 class RsSerialiser
 {
-	public:
+public:
+	/** Remember that every pqistreamer allocates an input buffer of this size!
+	 * So don't make it too big! */
+	static constexpr uint32_t MAX_SERIAL_SIZE = 262143; /* 2^18 -1 */
+
 	RsSerialiser();
 	~RsSerialiser();
 	bool        addSerialType(RsSerialType *type);
@@ -76,7 +80,7 @@ class RsSerialiser
 	RsItem *    deserialise(void *data, uint32_t *size);
 	
 	
-	private:
+private:
 	std::map<uint32_t, RsSerialType *> serialisers;
 };
 
@@ -95,6 +99,8 @@ uint16_t  getRsItemService(uint32_t type);
 
 /* size constants */
 uint32_t getRsPktBaseSize();
+
+RS_DEPRECATED_FOR(RsSerialiser::MAX_SERIAL_SIZE)
 uint32_t getRsPktMaxSize();
 
 
@@ -106,5 +112,3 @@ std::ostream &printRsItemEnd(std::ostream &o, std::string n, uint16_t i);
 /* defined in rstlvtypes.cc - redeclared here for ease */
 std::ostream &printIndent(std::ostream &out, uint16_t indent);
 /* Wrapper class for data that is serialised somewhere else */
-
-#endif /* RS_BASE_SERIALISER_H */

libretroshare/src/serialiser/rstypeserializer.h

@@ -480,7 +480,7 @@ struct RsTypeSerializer
 
 		switch(j)
 		{
-		case RsGenericSerializer::SIZE_ESTIMATE: // [[falltrough]]
+		case RsGenericSerializer::SIZE_ESTIMATE: // [[falltrough]];
 		case RsGenericSerializer::SERIALIZE:
 		{
 			uint32_t aSize = member.size();
@@ -499,10 +499,11 @@ struct RsTypeSerializer
 			if(!ctx.mOk) break;
 
 			/* This check is not perfect but will catch most pathological cases.
-			 * Avoid multiplying by sizeof(el_t) as it is not a good exitimation
+			 * Avoid multiplying by sizeof(el_t) as it is not a good estimation
 			 * of the actual serialized size, depending on the elements
-			 * structure and on the so it would raises many false positives. */
+			 * structure and on the so it would raises many false positives.
-			if(elCount > ctx.mSize - ctx.mOffset)
+			 * Arithmetic operations on elCount are also at risk of overflow */
+			if(elCount > RsSerialiser::MAX_SERIAL_SIZE)
 			{
 				ctx.mOk = false;
 				RsErr() << __PRETTY_FUNCTION__ << " attempt to deserialize a "
@@ -626,7 +627,7 @@ struct RsTypeSerializer
 		{
 			uint32_t len = static_cast<uint32_t>(member.length());
 			RS_SERIAL_PROCESS(len);
-			if(len > ctx.mSize - ctx.mOffset)
+			if(len + ctx.mOffset > ctx.mSize)
 			{
 				RsErr() << __PRETTY_FUNCTION__ << std::errc::no_buffer_space
 				        << std::endl;
@@ -641,19 +642,33 @@ struct RsTypeSerializer
 			uint32_t len;
 			RS_SERIAL_PROCESS(len);
 			if(!ctx.mOk) break;
-			if(len > ctx.mSize - ctx.mOffset)
+
+			if(len > RsSerialiser::MAX_SERIAL_SIZE)
 			{
 				ctx.mOk = false;
 				RsErr() << __PRETTY_FUNCTION__ << " attempt to deserialize a "
-				        << "string with apparently malformed elements count."
+				        << "string with apparently malformed length."
 				        << " len: " << len
 				        << " ctx.mSize: " << ctx.mSize
 				        << " ctx.mOffset: " << ctx.mOffset << " "
-				        << std::errc::argument_out_of_domain
+				        << std::errc::argument_out_of_domain << std::endl;
-				        << std::endl;
 				print_stacktrace();
 				break;
 			}
+
+			if(len + ctx.mOffset > ctx.mSize)
+			{
+				ctx.mOk = false;
+				RsErr() << __PRETTY_FUNCTION__ << " attempt to deserialize a "
+				        << "string with a length bigger available data."
+				        << " len: " << len
+				        << " ctx.mSize: " << ctx.mSize
+				        << " ctx.mOffset: " << ctx.mOffset << " "
+				        << std::errc::no_buffer_space << std::endl;
+				print_stacktrace();
+				break;
+			}
+
 			member.resize(len);
 			memcpy(&member[0], ctx.mData + ctx.mOffset, len);
 			ctx.mOffset += len;