fix(NODE-6536): Binary.read never returns number[] and reads beyond content (#727)

Author: nbbeeken

src/binary.ts

@@ -61,9 +61,48 @@ export class Binary extends BSONValue {
   /** User BSON type */
   static readonly SUBTYPE_USER_DEFINED = 128;
 
-  buffer!: Uint8Array;
-  sub_type!: number;
-  position!: number;
+  /**
+   * The bytes of the Binary value.
+   *
+   * The format of a Binary value in BSON is defined as:
+   * ```txt
+   * binary	::= int32 subtype (byte*)
+   * ```
+   *
+   * This `buffer` is the "(byte*)" segment.
+   *
+   * Unless the value is subtype 2, then deserialize will read the first 4 bytes as an int32 and set this to the remaining bytes.
+   *
+   * ```txt
+   * binary	::= int32 unsigned_byte(2) int32 (byte*)
+   * ```
+   *
+   * @see https://bsonspec.org/spec.html
+   */
+  public buffer: Uint8Array;
+  /**
+   * The binary subtype.
+   *
+   * Current defined values are:
+   *
+   * - `unsigned_byte(0)` Generic binary subtype
+   * - `unsigned_byte(1)` Function
+   * - `unsigned_byte(2)` Binary (Deprecated)
+   * - `unsigned_byte(3)` UUID (Deprecated)
+   * - `unsigned_byte(4)` UUID
+   * - `unsigned_byte(5)` MD5
+   * - `unsigned_byte(6)` Encrypted BSON value
+   * - `unsigned_byte(7)` Compressed BSON column
+   * - `unsigned_byte(8)` Sensitive
+   * - `unsigned_byte(9)` Vector
+   * - `unsigned_byte(128)` - `unsigned_byte(255)` User defined
+   */
+  public sub_type: number;
+  /**
+   * The Binary's `buffer` can be larger than the Binary's content.
+   * This property is used to determine where the content ends in the buffer.
+   */
+  public position: number;
 
   /**
    * Create a new Binary instance.
@@ -160,16 +199,15 @@ export class Binary extends BSONValue {
   }
 
   /**
-   * Reads **length** bytes starting at **position**.
+   * Returns a view of **length** bytes starting at **position**.
    *
    * @param position - read from the given position in the Binary.
    * @param length - the number of bytes to read.
    */
-  read(position: number, length: number): BinarySequence {
+  read(position: number, length: number): Uint8Array {
     length = length && length > 0 ? length : this.position;
-
-    // Let's return the data based on the type we have
-    return this.buffer.slice(position, position + length);
+    const end = position + length;
+    return this.buffer.subarray(position, end > this.position ? this.position : end);
   }
 
   /** returns a view of the binary value as a Uint8Array */
@@ -219,7 +257,7 @@ export class Binary extends BSONValue {
 
   toUUID(): UUID {
     if (this.sub_type === Binary.SUBTYPE_UUID) {
-      return new UUID(this.buffer.slice(0, this.position));
+      return new UUID(this.buffer.subarray(0, this.position));
     }
 
     throw new BSONError(

src/parser/deserializer.ts

@@ -296,7 +296,7 @@ function deserializeObject(
 
       // We have a raw value
       if (raw) {
-        value = buffer.slice(index, index + objectSize);
+        value = buffer.subarray(index, index + objectSize);
       } else {
         let objectOptions = options;
         if (!globalUTFValidation) {
@@ -374,52 +374,24 @@ function deserializeObject(
       if (binarySize > buffer.byteLength)
         throw new BSONError('Binary type size larger than document size');
 
-      // Decode as raw Buffer object if options specifies it
-      if (buffer['slice'] != null) {
-        // If we have subtype 2 skip the 4 bytes for the size
-        if (subType === Binary.SUBTYPE_BYTE_ARRAY) {
-          binarySize = NumberUtils.getInt32LE(buffer, index);
-          index += 4;
-          if (binarySize < 0)
-            throw new BSONError('Negative binary type element size found for subtype 0x02');
-          if (binarySize > totalBinarySize - 4)
-            throw new BSONError('Binary type with subtype 0x02 contains too long binary size');
-          if (binarySize < totalBinarySize - 4)
-            throw new BSONError('Binary type with subtype 0x02 contains too short binary size');
-        }
+      // If we have subtype 2 skip the 4 bytes for the size
+      if (subType === Binary.SUBTYPE_BYTE_ARRAY) {
+        binarySize = NumberUtils.getInt32LE(buffer, index);
+        index += 4;
+        if (binarySize < 0)
+          throw new BSONError('Negative binary type element size found for subtype 0x02');
+        if (binarySize > totalBinarySize - 4)
+          throw new BSONError('Binary type with subtype 0x02 contains too long binary size');
+        if (binarySize < totalBinarySize - 4)
+          throw new BSONError('Binary type with subtype 0x02 contains too short binary size');
+      }
 
-        if (promoteBuffers && promoteValues) {
-          value = ByteUtils.toLocalBufferType(buffer.slice(index, index + binarySize));
-        } else {
-          value = new Binary(buffer.slice(index, index + binarySize), subType);
-          if (subType === constants.BSON_BINARY_SUBTYPE_UUID_NEW && UUID.isValid(value)) {
-            value = value.toUUID();
-          }
-        }
+      if (promoteBuffers && promoteValues) {
+        value = ByteUtils.toLocalBufferType(buffer.subarray(index, index + binarySize));
       } else {
-        // If we have subtype 2 skip the 4 bytes for the size
-        if (subType === Binary.SUBTYPE_BYTE_ARRAY) {
-          binarySize = NumberUtils.getInt32LE(buffer, index);
-          index += 4;
-          if (binarySize < 0)
-            throw new BSONError('Negative binary type element size found for subtype 0x02');
-          if (binarySize > totalBinarySize - 4)
-            throw new BSONError('Binary type with subtype 0x02 contains too long binary size');
-          if (binarySize < totalBinarySize - 4)
-            throw new BSONError('Binary type with subtype 0x02 contains too short binary size');
-        }
-
-        if (promoteBuffers && promoteValues) {
-          value = ByteUtils.allocateUnsafe(binarySize);
-          // Copy the data
-          for (i = 0; i < binarySize; i++) {
-            value[i] = buffer[index + i];
-          }
-        } else {
-          value = new Binary(buffer.slice(index, index + binarySize), subType);
-          if (subType === constants.BSON_BINARY_SUBTYPE_UUID_NEW && UUID.isValid(value)) {
-            value = value.toUUID();
-          }
+        value = new Binary(buffer.subarray(index, index + binarySize), subType);
+        if (subType === constants.BSON_BINARY_SUBTYPE_UUID_NEW && UUID.isValid(value)) {
+          value = value.toUUID();
         }
       }
 

test/node/binary.test.ts

@@ -1,6 +1,6 @@
 import { expect } from 'chai';
 import * as vm from 'node:vm';
-import { Binary, BSON } from '../register-bson';
+import { __isWeb__, Binary, BSON } from '../register-bson';
 import * as util from 'node:util';
 
 describe('class Binary', () => {
@@ -81,6 +81,46 @@ describe('class Binary', () => {
     });
   });
 
+  describe('read()', function () {
+    const LocalBuffer = __isWeb__ ? Uint8Array : Buffer;
+
+    it('reads a single byte from the buffer', function () {
+      const binary = new Binary();
+      binary.put(0x42);
+      expect(binary.read(0, 1)).to.deep.equal(LocalBuffer.of(0x42));
+    });
+
+    it('does not read beyond binary.position', function () {
+      const binary = new Binary();
+      binary.put(0x42);
+      expect(binary.buffer.byteLength).to.equal(256);
+      expect(binary.read(0, 10)).to.deep.equal(LocalBuffer.of(0x42));
+    });
+
+    it('reads a single byte from the buffer at the given position', function () {
+      const binary = new Binary();
+      binary.put(0x42);
+      binary.put(0x43);
+      binary.put(0x44);
+      expect(binary.read(1, 1)).to.deep.equal(LocalBuffer.of(0x43));
+    });
+
+    it('reads nothing if the position is out of bounds', function () {
+      const binary = new Binary();
+      expect(binary.read(1, 0)).to.have.lengthOf(0);
+    });
+
+    it('sets length to position if not provided', function () {
+      const binary = new Binary();
+      binary.put(0x42);
+      binary.put(0x42);
+      binary.put(0x42);
+      expect(binary.position).to.equal(3);
+      // @ts-expect-error: checking behavior TS doesn't support
+      expect(binary.read(0)).to.have.lengthOf(3);
+    });
+  });
+
   context('inspect()', () => {
     it(`when value is default returns "Binary.createFromBase64('', 0)"`, () => {
       expect(util.inspect(new Binary())).to.equal(`Binary.createFromBase64('', 0)`);
@@ -93,6 +133,7 @@ describe('class Binary', () => {
     });
 
     it(`when value is default with a subtype returns "Binary.createFromBase64('', 35)"`, () => {
+      // @ts-expect-error: check null is handled the same as undefined
       expect(util.inspect(new Binary(null, 0x23))).to.equal(`Binary.createFromBase64('', 35)`);
     });
 

test/types/bson.test-d.ts

@@ -85,3 +85,5 @@ expectNotDeprecated(new ObjectId(42 as string | number));
 
 // Timestamp accepts timestamp because constructor allows: {i:number, t:number}
 new Timestamp(new Timestamp(0n))
+
+expectType<(position: number, length: number) => Uint8Array>(Binary.prototype.read);