diff --git a/src/packets/Serializable.ts b/src/packets/Serializable.ts index 8f3d16e..656a815 100644 --- a/src/packets/Serializable.ts +++ b/src/packets/Serializable.ts @@ -99,6 +99,11 @@ 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. */ abstract serialize(buffer?: ArrayBuffer, byteOffset?: number): void; @@ -150,9 +155,13 @@ export abstract class Serializable { buffer: ArrayBuffer; byteOffset: number; byteLength: number; - // NOTE: ESlint absurdly complaining about "Expected indentation of 2 tabs but - // found 1". } { + 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(); diff --git a/src/test/packets/RTP/RtpPacket.test.ts b/src/test/packets/RTP/RtpPacket.test.ts index c6d2ebb..f1ea1ac 100644 --- a/src/test/packets/RTP/RtpPacket.test.ts +++ b/src/test/packets/RTP/RtpPacket.test.ts @@ -685,6 +685,23 @@ 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', () => { + packet.setSequenceNumber(55555); + expect(packet.needsSerialization()).toBe(false); + expect(packet.getSequenceNumber()).toBe(55555); + + const buffer = new ArrayBuffer(1500); + + packet.serialize(buffer, 0); + 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); + }); }); describe('clone packet into a given buffer', () => { diff --git a/src/test/packets/Serializable.test.ts b/src/test/packets/Serializable.test.ts index f9596b9..edff563 100644 --- a/src/test/packets/Serializable.test.ts +++ b/src/test/packets/Serializable.test.ts @@ -98,15 +98,26 @@ describe('parse Foo 1', () => { expect(areDataViewsEqual(foo.getView(), clonedView)).toBe(true); }); - test('serialize() fails if current buffer is given and it collides', () => { - foo.serialize(view.buffer, /* byteOffset */ 9); + test('serialize() succeeds if current buffer is given with a byteOffset that avoids collision', () => { + expect(() => foo.serialize(view.buffer, /* byteOffset */ 10)).not.toThrow(); expect(foo.getByteLength()).toBe(10); expect(foo.needsSerialization()).toBe(false); // This is true because obviously both are the same DataView instance, // however it's been overwritten. expect(areDataViewsEqual(foo.getView(), view)).toBe(true); - expect(areDataViewsEqual(foo.getView(), clonedView)).toBe(false); + expect(areDataViewsEqual(foo.getView(), clonedView)).toBe(true); + }); + + test('serialize() fails if current buffer is given with same byteOffset', () => { + expect(() => foo.serialize(view.buffer, /* byteOffset */ 0)).toThrow(Error); + + expect(foo.getByteLength()).toBe(10); + expect(foo.needsSerialization()).toBe(false); + // This is true because obviously both are the same DataView instance, + // however it's been overwritten. + expect(areDataViewsEqual(foo.getView(), view)).toBe(true); + expect(areDataViewsEqual(foo.getView(), clonedView)).toBe(true); }); test('clone() succeeds', () => {