fix: updating readElement to match underlying data structure (aws#215)

The readElement function was previously not taking advantage of
the underlying data structure, which is a sequence of elements,
each of which is a sequence of fields. This resulted in several
superfluous calls to built-in functions (e.g. Array.prototype.splice).

This commit adds an additional parameter to readElement, which is
the number of fields per element, and adapts the code accordingly.
The return of readElement is now an array of arrays, which simplifies
its manipulation by the two caller functions.

Author: PetarMax

modules/serialize/src/deserialize_factory.ts

@@ -88,9 +88,7 @@ export function deserializeFactory<Suite extends AlgorithmSuite> (
      */
     if (22 + contextLength > dataView.byteLength) return false // not enough data
 
-    const encryptionContext = contextLength > 0
-      ? decodeEncryptionContext(messageBuffer.slice(22, 22 + contextLength))
-      : {}
+    const encryptionContext = decodeEncryptionContext(messageBuffer.slice(22, 22 + contextLength))
     const dataKeyInfo = deserializeEncryptedDataKeys(messageBuffer, 22 + contextLength)
 
     /* Check for early return (Postcondition): Not Enough Data. deserializeEncryptedDataKeys will return false if it does not have enough data.
@@ -176,23 +174,20 @@ export function deserializeFactory<Suite extends AlgorithmSuite> (
     /* Precondition: There must be at least 1 EncryptedDataKey element. */
     needs(encryptedDataKeysCount, 'No EncryptedDataKey found.')
 
-    const elementInfo = readElements(encryptedDataKeysCount * 3, buffer, startPos + 2)
+    const elementInfo = readElements(encryptedDataKeysCount, 3, buffer, startPos + 2)
     /* Check for early return (Postcondition): readElement will return false if there is not enough data.
      * I can only continue if I have at least the entire EDK section.
      */
     if (!elementInfo) return false
     const { elements, readPos } = elementInfo
 
-    let remainingKeyCount = encryptedDataKeysCount
-    const encryptedDataKeys = []
-    while (remainingKeyCount--) {
-      const [rawId, rawInfo] = elements.splice(0, 2)
-      const [providerId, providerInfo] = [rawId, rawInfo].map(toUtf8)
-      const [encryptedDataKey] = elements.splice(0, 1)
-      // The providerInfo is technically a binary field, so I must pass rawInfo
-      const edk = new EncryptedDataKey({ providerInfo, providerId, encryptedDataKey, rawInfo })
-      encryptedDataKeys.push(edk)
-    }
+    const encryptedDataKeys = elements.map(
+      ([rawId, rawInfo, encryptedDataKey], _) => {
+        const providerId = toUtf8(rawId)
+        const providerInfo = toUtf8(rawInfo)
+        return new EncryptedDataKey({ providerInfo, providerId, encryptedDataKey, rawInfo })
+      }
+    )
     Object.freeze(encryptedDataKeys)
     return { encryptedDataKeys, readPos }
   }
@@ -219,7 +214,7 @@ export function deserializeFactory<Suite extends AlgorithmSuite> (
       encodedEncryptionContext.byteLength
     )
     const pairsCount = dataView.getUint16(0, false) // big endian
-    const elementInfo = readElements(pairsCount * 2, encodedEncryptionContext, 2)
+    const elementInfo = readElements(pairsCount, 2, encodedEncryptionContext, 2)
     /* Postcondition: Since the encryption context has a length, it must have pairs.
      * Unlike the encrypted data key section, the encryption context has a length
      * element.  This means I should always pass the entire section.
@@ -230,9 +225,8 @@ export function deserializeFactory<Suite extends AlgorithmSuite> (
     /* Postcondition: The byte length of the encodedEncryptionContext must match the readPos. */
     needs(encodedEncryptionContext.byteLength === readPos, 'Overflow, too much data.')
 
-    let count = pairsCount
-    while (count--) {
-      const [key, value] = elements.splice(0, 2).map(toUtf8)
+    for (let count = 0; count < pairsCount; count++) {
+      const [key, value] = elements[count].map(toUtf8)
       /* Postcondition: The number of keys in the encryptionContext must match the pairsCount.
        * If the same Key value is serialized...
        */

modules/serialize/src/read_element.ts

@@ -43,6 +43,7 @@ import { needs } from '@aws-crypto/material-management'
  */
 export function readElements (
   elementCount: number,
+  fieldsPerElement: number,
   buffer: Uint8Array,
   readPos: number = 0
 ) {
@@ -59,22 +60,27 @@ export function readElements (
   )
   const elements = []
 
-  /* Precondition: readElements readPos must be within the byte length of the buffer given. */
+  /* Precondition: readPos must be non-negative and within the byte length of the buffer given. */
   needs(readPos >= 0 && dataView.byteLength >= readPos, 'readPos out of bounds.')
 
-  /* Precondition: elementCount must not be negative. */
-  needs(elementCount >= 0, 'elementCount must be positive.')
+  /* Precondition: elementCount and fieldsPerElement must be non-negative. */
+  needs(elementCount >= 0 && fieldsPerElement >= 0, 'elementCount and fieldsPerElement must be positive.')
 
   while (elementCount--) {
-    /* Check for early return (Postcondition): Enough data must exist to read the Uint16 length value. */
-    if (readPos + 2 > dataView.byteLength) return false
-    const length = dataView.getUint16(readPos, false) // big endian
-    readPos += 2
-    /* Check for early return (Postcondition): Enough data must exist length of the value. */
-    if (readPos + length > dataView.byteLength) return false
-    const elementBinary = buffer.slice(readPos, readPos + length)
-    elements.push(elementBinary)
-    readPos += length
+    let element = []
+    let fieldCount = fieldsPerElement
+    while (fieldCount--) {
+      /* Check for early return (Postcondition): Enough data must exist to read the Uint16 length value. */
+      if (readPos + 2 > dataView.byteLength) return false
+      const length = dataView.getUint16(readPos, false) // big endian
+      readPos += 2
+      /* Check for early return (Postcondition): Enough data must exist length of the value. */
+      if (readPos + length > dataView.byteLength) return false
+      const fieldBinary = buffer.slice(readPos, readPos + length)
+      element.push(fieldBinary)
+      readPos += length
+    }
+    elements.push(element)
   }
   return { elements, readPos }
 }

modules/serialize/test/read_elements.test.ts

@@ -22,23 +22,14 @@ import { concatBuffers } from '../src/concat_buffers'
 import { Buffer } from 'buffer'
 import * as fixtures from './fixtures'
 
-describe('readElements', () => {
-  it('should be able to find one element', () => {
-    const len = Buffer.alloc(2)
-    len.writeUInt16BE(4, 0)
-    const data = new Uint8Array([...Buffer.from('data')])
-    const buff = concatBuffers(len, data)
-    const info = readElements(1, buff, 0)
-    if (info === false) throw new Error('Fail')
-    expect(info.elements).to.be.instanceof(Array)
-    expect(info.elements.length).to.eql(1)
-    expect(info.elements[0]).to.deep.equal(data)
-    expect(info.readPos).to.eql(6)
-  })
+function randomNat (limit : number) : number {
+  return Math.floor(Math.random() * limit)
+}
 
-  it('should be able to handle multiple elements', () => {
+describe('readElements', () => {
+  it('should be able to handle multiple elements containing multiple fields without padding', () => {
     const utf8DataStrings = [
-      'some utf8 information', '\u00bd + \u00bc = \u00be', 'to encode'
+      'here', 'is', 'some', 'utf8', 'information', '\u00bd + \u00bc = \u00be'
     ]
     const buffData = utf8DataStrings.map(str => new Uint8Array([...Buffer.from(str)]))
     const buff = concatBuffers(...buffData.map(bufStr => {
@@ -47,75 +38,93 @@ describe('readElements', () => {
       return concatBuffers(len, bufStr)
     }))
 
-    const info = readElements(3, buff, 0)
-
-    if (info === false) throw new Error('Fail')
-    expect(info.elements).to.be.instanceof(Array)
-    expect(info.elements.length).to.eql(3)
-    expect(info.elements[0]).to.deep.equal(buffData[0])
-    expect(info.elements[1]).to.deep.equal(buffData[1])
-    expect(info.elements[2]).to.deep.equal(buffData[2])
-    expect(Buffer.from(info.elements[0]).toString()).to.deep.equal(utf8DataStrings[0])
-    expect(Buffer.from(info.elements[1]).toString()).to.deep.equal(utf8DataStrings[1])
-    expect(Buffer.from(info.elements[2]).toString()).to.deep.equal(utf8DataStrings[2])
-    expect(info.readPos).to.eql(48)
+    /* The elements in the buffer can be arranged in several ways.
+     * For example, we can think of them as six elements with one
+     * field each, or as three elements with two fields each.
+     */
+    const dimensions = [[1, 6], [2, 3], [3, 2], [6, 1]]
+
+    dimensions.map(([elementCount, fieldsPerElement]) => {
+      const info = readElements(elementCount, fieldsPerElement, buff)
+
+      if (info === false) throw new Error('Fail')
+      let elements = info.elements
+      expect(elements).to.be.instanceof(Array)
+      expect(elements.length).to.eql(elementCount)
+
+      for (let eCount = 0; eCount < elementCount; eCount++) {
+        let element = info.elements[eCount]
+        expect(element).to.be.instanceof(Array)
+        expect(element.length).to.eql(fieldsPerElement)
+        for (let fCount = 0; fCount < fieldsPerElement; fCount++) {
+          let field = element[fCount]
+          expect(field).to.deep.equal(buffData[eCount * fieldsPerElement + fCount])
+          expect(Buffer.from(field).toString()).to.deep.equal(utf8DataStrings[eCount * fieldsPerElement + fCount])
+        }
+      }
+
+      expect(info.readPos).to.eql(buff.byteLength)
+    })
   })
 
-  it('should be able to handle multiple elements when not reading from the beginning', () => {
-    const utf8DataStrings = [
-      'some utf8 information', '\u00bd + \u00bc = \u00be', 'to encode'
-    ]
+  it('should be able to handle multiple elements containing multiple fields with various padding', () => {
+    let numberOfRuns = 16
+    const maxPaddingLength = 1024
+    const utf8DataStrings = [ 'here', 'is', 'some', 'utf8', 'information', '\u00bd + \u00bc = \u00be' ]
     const buffData = utf8DataStrings.map(str => new Uint8Array([...Buffer.from(str)]))
-    const padding = Buffer.alloc(10)
-    const buff = concatBuffers(padding, ...buffData.map(bufStr => {
+    const mainBuffer = concatBuffers(...buffData.map(bufStr => {
       const len = Buffer.alloc(2)
       len.writeUInt16BE(bufStr.byteLength, 0)
       return concatBuffers(len, bufStr)
     }))
 
-    const info = readElements(3, buff, padding.length)
-
-    if (info === false) throw new Error('Fail')
-    expect(info.elements).to.be.instanceof(Array)
-    expect(info.elements.length).to.eql(3)
-    expect(info.elements[0]).to.deep.equal(buffData[0])
-    expect(info.elements[1]).to.deep.equal(buffData[1])
-    expect(info.elements[2]).to.deep.equal(buffData[2])
-    expect(Buffer.from(info.elements[0]).toString()).to.deep.equal(utf8DataStrings[0])
-    expect(Buffer.from(info.elements[1]).toString()).to.deep.equal(utf8DataStrings[1])
-    expect(Buffer.from(info.elements[2]).toString()).to.deep.equal(utf8DataStrings[2])
-    expect(info.readPos).to.eql(58)
+    const dimensions = [[1, 6], [2, 3], [3, 2], [6, 1]]
+
+    while (numberOfRuns--) {
+      let leftPadding = Buffer.alloc(randomNat(maxPaddingLength))
+      let rightPadding = Buffer.alloc(randomNat(maxPaddingLength))
+      let buff = concatBuffers(leftPadding, mainBuffer, rightPadding)
+
+      dimensions.map(([elementCount, fieldsPerElement]) => {
+        const info = readElements(elementCount, fieldsPerElement, buff, leftPadding.byteLength)
+
+        if (info === false) throw new Error('Fail')
+        let elements = info.elements
+        expect(elements).to.be.instanceof(Array)
+        expect(elements.length).to.eql(elementCount)
+
+        for (let eCount = 0; eCount < elementCount; eCount++) {
+          let element = info.elements[eCount]
+          expect(element).to.be.instanceof(Array)
+          expect(element.length).to.eql(fieldsPerElement)
+          for (let fCount = 0; fCount < fieldsPerElement; fCount++) {
+            let field = element[fCount]
+            expect(field).to.deep.equal(buffData[eCount * fieldsPerElement + fCount])
+            expect(Buffer.from(field).toString()).to.deep.equal(utf8DataStrings[eCount * fieldsPerElement + fCount])
+          }
+        }
+
+        expect(info.readPos).to.eql(leftPadding.byteLength + mainBuffer.byteLength)
+      })
+    }
   })
 
-  it('should be able to handle multiple elements with padding on both sides', () => {
-    const utf8DataStrings = [
-      'some utf8 information', '\u00bd + \u00bc = \u00be', 'to encode'
-    ]
-    const buffData = utf8DataStrings.map(str => new Uint8Array([...Buffer.from(str)]))
-    const padding = Buffer.alloc(10)
-    const buff = concatBuffers(padding, ...buffData.map(bufStr => {
-      const len = Buffer.alloc(2)
-      len.writeUInt16BE(bufStr.byteLength, 0)
-      return concatBuffers(len, bufStr)
-    }), padding)
-
-    const info = readElements(3, buff, padding.length)
-
-    if (info === false) throw new Error('Fail')
-    expect(info.elements).to.be.instanceof(Array)
-    expect(info.elements.length).to.eql(3)
-    expect(info.elements[0]).to.deep.equal(buffData[0])
-    expect(info.elements[1]).to.deep.equal(buffData[1])
-    expect(info.elements[2]).to.deep.equal(buffData[2])
-    expect(Buffer.from(info.elements[0]).toString()).to.deep.equal(utf8DataStrings[0])
-    expect(Buffer.from(info.elements[1]).toString()).to.deep.equal(utf8DataStrings[1])
-    expect(Buffer.from(info.elements[2]).toString()).to.deep.equal(utf8DataStrings[2])
-    expect(info.readPos).to.eql(58)
+  it('Precondition: readPos must be non-negative and within the byte length of the buffer given.', () => {
+    const buff = new Uint8Array(32)
+    const readPosBeyondBuff = buff.byteLength + 1
+    expect(() => readElements(1, 1, buff, readPosBeyondBuff)).to.throw()
+  })
+
+  it('Precondition: elementCount and fieldsPerElement must be non-negative.', () => {
+    const buff = new Uint8Array(32)
+    expect(() => readElements(-1, 1, buff)).to.throw()
+    expect(() => readElements(1, -1, buff)).to.throw()
+    expect(() => readElements(-1, -1, buff)).to.throw()
   })
 
   it('Check for early return (Postcondition): Enough data must exist to read the Uint16 length value.; Check for early return (Postcondition): Enough data must exist length of the value.', () => {
     const utf8DataStrings = [
-      'some utf8 information', '\u00bd + \u00bc = \u00be', 'to encode'
+      'here', 'is', 'some', 'utf8', 'information', '\u00bd + \u00bc = \u00be'
     ]
     const buffData = utf8DataStrings.map(str => new Uint8Array([...Buffer.from(str)]))
     const buff = concatBuffers(...buffData.map(bufStr => {
@@ -124,35 +133,13 @@ describe('readElements', () => {
       return concatBuffers(len, bufStr)
     }))
 
-    // By testing every combination of byte length possible
-    // Both loop invariants are covered.
-    for (let i = 0; buff.byteLength > i; i++) {
-      const info = readElements(3, buff.slice(0, i), 0)
-      expect(info).to.eql(false)
-    }
-    // Engage pedantry...
-    const info = readElements(3, buff.slice(0, buff.byteLength), 0)
-    if (info === false) throw new Error('Fail')
-    expect(info.elements).to.be.instanceof(Array)
-    expect(info.elements.length).to.eql(3)
-    expect(info.elements[0]).to.deep.equal(buffData[0])
-    expect(info.elements[1]).to.deep.equal(buffData[1])
-    expect(info.elements[2]).to.deep.equal(buffData[2])
-    expect(Buffer.from(info.elements[0]).toString()).to.deep.equal(utf8DataStrings[0])
-    expect(Buffer.from(info.elements[1]).toString()).to.deep.equal(utf8DataStrings[1])
-    expect(Buffer.from(info.elements[2]).toString()).to.deep.equal(utf8DataStrings[2])
-    expect(info.readPos).to.eql(48)
-  })
-
-  it('Precondition: readElements readPos must be within the byte length of the buffer given.', () => {
-    const buff = new Uint8Array(32)
-    const readPosBeyondBuff = buff.byteLength + 1
-    expect(() => readElements(1, buff, readPosBeyondBuff)).to.throw()
-  })
+    /* Will return false when trying to read the length of the seventh element */
+    const infoFalse1 = readElements(1, 7, buff)
+    expect(infoFalse1).to.equal(false)
 
-  it('Precondition: elementCount must not be negative.', () => {
-    const buff = new Uint8Array(32)
-    expect(() => readElements(-1, buff)).to.throw()
+    /* Will return false when trying to read the sixth element */
+    const infoFalse2 = readElements(1, 6, buff.slice(0, buff.byteLength - 1))
+    expect(infoFalse2).to.equal(false)
   })
 
   it('ArrayBuffer for a Uint8Array or Buffer may be larger than the Uint8Array or Buffer that it is a view over is.', () => {
@@ -162,12 +149,12 @@ describe('readElements', () => {
      * read position past the length and count should succeed.
      */
     const buff = fixtures.basicEncryptionContext()
-    expect(readElements(4, buff, 0)).to.equal(false)
-    expect(readElements(4, buff, 4)).to.not.equal(false)
+    expect(readElements(4, 1, buff, 0)).to.equal(false)
+    expect(readElements(4, 1, buff, 4)).to.not.equal(false)
     /* Given this I can use this to construct a new view of part of the
      * ArrayBuffer to simulate a large ArrayBuffer that is sliced
      * into parts for efficiency. */
     const sharingArrayBuffer = new Uint8Array(buff.buffer, 4, buff.byteLength - 4)
-    expect(readElements(4, sharingArrayBuffer)).to.not.equal(false)
+    expect(readElements(4, 1, sharingArrayBuffer)).to.not.equal(false)
   })
 })