Improve serialize

src/packets/Serializable.ts

@@ -99,11 +99,10 @@ export abstract class Serializable {
 	 * @throws
 	 * - If serialization fails due to invalid content previously added.
 	 * - If given `buffer` doesn't have space enough to serialize the content.
-	 * - If the `buffer` member of the given `ArrayBuffer` and the given
-	 *   `byteOffset` match the `buffer` member and the `byteOffset` of the current
-	 *   view. The same buffer can be given but, if so, care must be taken with
-	 *   the valud of `byteOffset` to avoid data corruption if the serializetion
-	 *   happens in the same bytes where the packet data is currently placed.
+	 * - If the `buffer` member of the given `ArrayBuffer` is the same than the
+	 *   internal buffer in this packet and the given `byteOffset` would make
+	 *   the serialization happen in bytes currently used by the packet (this would
+	 *   corrupt the packet).
 	 */
 	abstract serialize(buffer?: ArrayBuffer, byteOffset?: number): void;
 
@@ -149,21 +148,13 @@ export abstract class Serializable {
 	 * whether the serialized content can fit into it and will throw otherwise.
 	 */
 	protected getSerializationBuffer(
-		buffer?: ArrayBuffer,
-		byteOffset?: number
+		buffer: ArrayBuffer | undefined,
+		byteOffset: number = 0
 	): {
 		buffer: ArrayBuffer;
 		byteOffset: number;
 		byteLength: number;
 	} {
-		if (buffer === this.view.buffer && byteOffset === this.view.byteOffset) {
-			throw new Error(
-				'given buffer cannot be the the same as the internal buffer of the packet'
-			);
-		}
-
-		byteOffset ??= 0;
-
 		const byteLength = this.getByteLength();
 
 		if (buffer) {
@@ -175,6 +166,25 @@ export abstract class Serializable {
 				);
 			}
 
+			// If the given buffer is the same as the one internally used by the view
+			// of this packet, we must assert that the new serialization would not
+			// happen in bytes where the packet currently exists.
+			// Here we must take into account the given byte offset and the byte
+			// offset of the view, the current packet length and the expected packet
+			// length after the serialization.
+			if (
+				buffer === this.view.buffer &&
+				((byteOffset >= this.view.byteOffset &&
+					byteOffset <= this.view.byteOffset + this.view.byteLength - 1) ||
+					(byteOffset + byteLength - 1 >= this.view.byteOffset &&
+						byteOffset + byteLength <=
+							this.view.byteOffset + this.view.byteLength - 1))
+			) {
+				throw new RangeError(
+					'given buffer is the same as the internal buffer of the packet and given byteLength would make serialization override existing packet data'
+				);
+			}
+
 			const uint8Array = new Uint8Array(buffer, byteOffset, byteLength);
 
 			// If a buffer is given, ensure the required length is filled with zeroes.
@@ -188,8 +198,8 @@ export abstract class Serializable {
 	}
 
 	protected cloneInternal(
-		buffer?: ArrayBuffer,
-		byteOffset?: number,
+		buffer: ArrayBuffer | undefined,
+		byteOffset: number = 0,
 		serializationBuffer?: ArrayBuffer,
 		serializationByteOffset?: number
 	): DataView {
@@ -202,8 +212,6 @@ export abstract class Serializable {
 		// If buffer is given, let's check whether it holds enough space for the
 		// content.
 		if (buffer) {
-			byteOffset ??= 0;
-
 			if (buffer.byteLength - byteOffset < this.view.byteLength) {
 				throw new RangeError(
 					`given buffer available space (${

src/test/packets/RTP/RtpPacket.test.ts

@@ -686,7 +686,7 @@ describe('serialize packet into a given buffer', () => {
 		expect(() => packet.serialize(buffer, byteOffset)).toThrow(RangeError);
 	});
 
-	test('serialization corrupts the packet if current buffer is given with a byteOffset that makes the serialization overlap the packet content', () => {
+	test('serialization fails if given buffer is the same as the internal one in the packet and given byteOffset would make serialization overlap the current packet content', () => {
 		packet.setSequenceNumber(55555);
 		expect(packet.needsSerialization()).toBe(false);
 		expect(packet.getSequenceNumber()).toBe(55555);
@@ -697,10 +697,8 @@ describe('serialize packet into a given buffer', () => {
 		expect(packet.getSequenceNumber()).toBe(55555);
 
 		// Here we are serializing the packet in its own buffer with an offset of 2,
-		// so we are effectively overriding its current content, including the bytes
-		// where the sequence number value is stored.
-		packet.serialize(buffer, 2);
-		expect(packet.getSequenceNumber()).not.toBe(55555);
+		// so we are effectively overriding its current content.
+		expect(() => packet.serialize(buffer, 2)).toThrow(RangeError);
 	});
 });
 

src/test/packets/Serializable.test.ts

@@ -145,18 +145,14 @@ describe('parse Foo 1', () => {
 
 		const buffer = new ArrayBuffer(10);
 		const byteOffset = 0;
-		const clonedFoo = foo.clone(
-			buffer,
-			byteOffset,
-			/* serializationBuffer */ view.buffer,
-			/* byteOffset */ 9
-		);
 
-		expect(clonedFoo.getByteLength()).toBe(10);
-		expect(clonedFoo.needsSerialization()).toBe(false);
-		// This is true because obviously both are the same DataView instance,
-		// however it's been overwritten.
-		expect(areDataViewsEqual(clonedFoo.getView(), view)).toBe(true);
-		expect(areDataViewsEqual(clonedFoo.getView(), clonedView)).toBe(false);
+		expect(() =>
+			foo.clone(
+				buffer,
+				byteOffset,
+				/* serializationBuffer */ view.buffer,
+				/* byteOffset */ 9
+			)
+		).toThrow(RangeError);
 	});
 });