Regression in v6.6.0 for parsing multi-byte length header that is split across write calls

[twegener-embertec]

The following commit altered parser.js _parseLength such that it generates an error state whenever _parseVarByteNum returns false:

commit 3eb494a19d6a5c855ab098ab6975a934b0bff00a
Author: Hans Klunder <[email protected]>
Date:   Tue Sep 8 11:59:32 2020 +0200

    Fix: restrict Variable Byte Integer to 24bits (#90)

That was to cater for the new max bytes check in _parseVarByteNum, that breaks the case where the remainder of the length header bytes are not available yet (this._list.length <= bytes), since that case is not an error, since it is just waiting for more bytes to be available.

It is relatively rare that it hits this problem, as it only happens if delayed incoming bytes occur at just the right (wrong) time, i.e. when parsing a multi-byte length header field. However, under high traffic we see this occurring a couple times a day since upgrading to mqtt-packet 6.6.0. I have tested winding back to 6.5.0 and we don't hit the problem there.

This is quite serious as when this occurs the byte stream does not get consumed properly, and after the first "Error: Invalid length" error occurs, parsing subsequent incoming bytes gets totally confused and emits numerous errors (losing the data), until the mqtt connection is dropped and re-established.

[twegener-embertec]

Here are some unit tests that demonstrate the problem (the first for a sanity check for the test approach, then the second revealing the bug). These tests pass on v6.5.0, but the latter test fails with v6.6.0:

test('split length publish longer', t => {
  t.plan(3)

  const length = 255
  const topic = 'test'
  const payloadLength = length - topic.length - 2

  const parser = mqtt.parser()
  const expected = {
    cmd: 'publish',
    retain: false,
    qos: 0,
    dup: false,
    length: length,
    topic: topic,
    payload: Buffer.from('a'.repeat(payloadLength))
  }

  parser.on('packet', packet => {
    t.deepLooseEqual(packet, expected, 'expected packet')
  })

  t.equal(parser.parse(Buffer.from([
    48, 255, 1, // Header
    0, topic.length, // Topic length
    116, 101, 115, 116 // Topic (test)
  ])), 6, 'remaining bytes')

  t.equal(parser.parse(Buffer.from(Array(payloadLength).fill(97))),
          0, 'remaining bytes')
})

test('split length parse', t => {
  t.plan(4)

  const length = 255
  const topic = 'test'
  const payloadLength = length - topic.length - 2

  const parser = mqtt.parser()
  const expected = {
    cmd: 'publish',
    retain: false,
    qos: 0,
    dup: false,
    length: length,
    topic: topic,
    payload: Buffer.from('a'.repeat(payloadLength))
  }

  parser.on('packet', packet => {
    t.deepLooseEqual(packet, expected, 'expected packet')
  })

  t.equal(parser.parse(Buffer.from([
    48, 255, // Header (partial length)
  ])), 1, 'remaining bytes')

  t.equal(parser.parse(Buffer.from([
    1,  // Rest of header length
    0, topic.length, // Topic length
    116, 101, 115, 116 // Topic (test)
  ])), 6, 'remaining bytes')

  t.equal(parser.parse(Buffer.from(Array(payloadLength).fill(97))),
          0, 'remaining bytes')
})

[twegener-embertec]

Introduced in #90

(adding this comment to get it to link to the other bug)

[mcollina]

Would you like to send a Pull Request to address this issue?

[twegener-embertec]

Here you go. :-)

[bkp7]

@YoDaMa, I think this issue should be closed