Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: restrict Variable Byte Integer to 24bits #90

Merged
merged 2 commits into from
Sep 8, 2020

Conversation

seriousme
Copy link
Contributor

@seriousme seriousme commented Sep 3, 2020

The MQTT 5.0 specification defines Variable Byte Integer as a maximum of 24 bits. The 3.x spec uses this format for packet length only but in 5.0 it is also used in properties (e.g. subscriptionIdentifier)

Testing generation of a Variable Byte Integer with anything bigger than that resulted in a Attempt to write outside buffer bounds error because numbers.js would allocate 0 bytes and still try to write 4 or more bytes.
Parsing of a Variable Byte Integer (e.g. in packet length) > 24 bits would not emit an error where it should.

This PR fixes that and adds tests to confirm 24 bits maximum.

It also renames the constants and variables used by the parser so they are inline with its intended use again.

Performance should not be impacted because:
a) generator algorithm has been simplified so it should be a tiny bit faster
b) the cache hides the performance of generation anyway :-)
c) the rest of the changes are only small logic updates

Kind regards,
Hans

@mcollina
Copy link
Member

mcollina commented Sep 3, 2020

Test fails on node 6.

Given we use the same logic for MQTT 3.1.1, would there be compatibility problems?

@seriousme
Copy link
Contributor Author

Turns out that in Node 6 handles Buffer.subarray different from later versions

Node 6:

> b= Buffer.from([1,2,3,4])
<Buffer 01 02 03 04>
> b.subarray(0,1)
Uint8Array [ 1 ]

Node 8 and later

> b= Buffer.from([1,2,3,4])
<Buffer 01 02 03 04>
> b.subarray(0,1)
<Buffer 01>

Buffer.slice works as well but creates a copy that needs to be allocated and thus costs some performance :-(
Solved now.

Kind regards,
Hans

@seriousme
Copy link
Contributor Author

seriousme commented Sep 3, 2020

As to compatibility :

MQTT 3.1, 3.11 and 5.0 all use the same algorithm with the same 24 bits max (decimal 268,435,455, hex 0x0f ff ff ff) but 3.1 and 3.11 only use it for length and do not name it Variable Byte Integer yet. So no compatibility impact :-)

Kind regards,
Hans

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants