Fix: error handling for Variable Byte Integer #95

[twegener-embertec]

This fixes the regression outlined in #95 which was introduced by #90 .
See #95 for more specific details of the primary problem being addressed here.

Changes:

• Avoid emitting false errors when not all bytes are present yet when parsing the variable byte integer for the length header.
• Emit error upon attempting to generate a variable byte integer beyond the maximum allowed size.
• Add more test coverage in this area.

[mcollina]

@YoDaMa could you take a look as well?

[mcollina]

lgtm

[YoDaMa]

shouldn't this be a less than or equal for maxBytes

[twegener-embertec]

I believe that the previous ' bytes < 5' was incorrect.

The maximum size of a var byte int is 4 bytes, so 4 bytes need to be examined in the loop.
The bytes variable starts at 0, so to examine 4 bytes, the loop has to be over 0, 1, 2, 3.
If it doesn't meet the ((current & constants.VARBYTEINT_FIN_MASK) === 0) condition for any of those four bytes, then that indicates that it has exceeded the maximum size, i.e. that bit being non-zero means that it is expecting more length bytes, but we are already at the maximum at for bytes === 3.

Here are a couple of tests that demonstrate that buggy aspect present in v6.6.0, failing to emit the required error there:

testParseError('Invalid length', Buffer.from(
  [16, 255, 255, 255, 255, 1]
), {})
testParseError('Invalid length', Buffer.from(
  [16, 255, 255, 255, 255, 127]
), {})

While the following tests do pass in v6.6.0:

testParseError('Invalid length', Buffer.from(
  [16, 255, 255, 255, 255, 128]
), {})
testParseError('Invalid length', Buffer.from(
  [16, 255, 255, 255, 255, 255, 1]
), {})

(Note that the error message is different between v6.6.0 and my changes, i.e. 'Invalid length' vs 'Invalid variable byte integer'.)

With the updated error message, the following tests all pass with my changes:

testParseError('Invalid variable byte integer', Buffer.from(
  [16, 255, 255, 255, 255]
), {})
testParseError('Invalid variable byte integer', Buffer.from(
  [16, 255, 255, 255, 128]
), {})
testParseError('Invalid variable byte integer', Buffer.from(
  [16, 255, 255, 255, 255, 1]
), {})
testParseError('Invalid variable byte integer', Buffer.from(
  [16, 255, 255, 255, 255, 127]
), {})
testParseError('Invalid variable byte integer', Buffer.from(
  [16, 255, 255, 255, 255, 128]
), {})
testParseError('Invalid variable byte integer', Buffer.from(
  [16, 255, 255, 255, 255, 255, 1]
), {})

That seems to be good evidence that my version has the correct while condition and that the previous one was another bug in v6.6.0.

[YoDaMa]

can you squash your commits down into 1 or 2 commits then I'll approve and merge @twegener-embertec

[twegener-embertec]

@YoDaMa can you please explain to me your rationale / guidelines / philosophy for squashing the commits?

I deliberately kept those commits separate since the purpose/effects of each are distinct:

• Add additional test coverage that still passes against the old code.
• Add test that exposes one pre-existing bug.
• Add test that exposes another pre-existing bug.
• Provide the fix.
• Restructure the test to deal with some tangentially related nodejs compatibility issue.

I'm reluctant to squash them as that loses that information, and also makes it harder to rewind and demonstrate the historical bugginess aspects.

I don't currently see any benefits in squashing them.

Perhaps you could do a --no-ff merge if you want the overall change to be obvious as well?

[YoDaMa]

@YoDaMa can you please explain to me your rationale / guidelines / philosophy for squashing the commits?

I deliberately kept those commits separate since the purpose/effects of each are distinct:

fair. I like that logic. The last commit seems extraneous beyond the earlier 5 commits. But it's good enough.

[twegener-embertec]

The last commit seems extraneous beyond the earlier 5 commits. But it's good enough.

Assuming you mean 909cbd3, that was added to give evidence in response to your concerns about the maxBytes threshold.
Ideally, I suppose that that commit should be reordered prior to the fix commit, so that it demonstrates that other bug before the fix.

If you want I could do that reordering via an interactive rebase, but I'd want to do that on a new branch to avoid screwing with this already public branch. I'm not sure how to integrate such a revised branch into this pull request, so if you want that please advise how. Otherwise I'll assume the "it's good enough" still stands.

[psorowka]

@mcollina is this already released and included in a current mqttjs release?