Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/packets/Serializable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand Down
17 changes: 17 additions & 0 deletions src/test/packets/RTP/RtpPacket.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
17 changes: 14 additions & 3 deletions src/test/packets/Serializable.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down