diff --git a/src/packets/Serializable.ts b/src/packets/Serializable.ts index 656a815..bb29356 100644 --- a/src/packets/Serializable.ts +++ b/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 (${ diff --git a/src/test/packets/RTP/RtpPacket.test.ts b/src/test/packets/RTP/RtpPacket.test.ts index f1ea1ac..1fa761e 100644 --- a/src/test/packets/RTP/RtpPacket.test.ts +++ b/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); }); }); diff --git a/src/test/packets/Serializable.test.ts b/src/test/packets/Serializable.test.ts index edff563..a7e935d 100644 --- a/src/test/packets/Serializable.test.ts +++ b/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); }); });