Skip to content

emcute: fix length field calculation#11958

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
miri64:emcute/fix/length-calculation
Oct 1, 2019
Merged

emcute: fix length field calculation#11958
miri64 merged 1 commit intoRIOT-OS:masterfrom
miri64:emcute/fix/length-calculation

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Aug 5, 2019

The length field in an MQTT packet carries the total length of the packet. If it is below 256 (i.e. fits in one byte) only one byte is used for the length field. If it is larger than that 3 bytes are used, with the first byte having the value 0x01 and the remaining bytes representing the length in as a 2 byte unsigned integer in network byte order. Resulting from that it can be assessed that the check in emcutes's set_len() function is wrong as it needs to be checked if len is lesser or equal to 0xff - 1 (-1 for the length field itself). len <= (0xff - 1) can be simplified to len < 0xff. For some larger packages this safes 2 bytes of wasted packet space.

Contribution description

Testing procedure

Merge #11823 and run its testing procedure.

Issues/PRs references

Split out of #11823 to stream-line the review.

The length field in an MQTT packet carries the _total_ length of the
packet. If it is below 256 (i.e. fits in one byte) only one byte is
used for the length field. If it is larger than that 3 bytes are used,
with the first byte having the value `0x01` and the remaining bytes
representing the length in as a 2 byte unsigned integer in network byte
order. Resulting from that it can be assessed that the check in
`emcutes`'s `set_len()` function is wrong as it needs to be checked if
`len` is lesser or equal to `0xff - 1`. `len <= (0xff - 1)` can be
simplified to `len < 0xff`. For some larger packages this safes 2 bytes
of wasted packet space.
@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Aug 5, 2019
@miri64 miri64 added this to the Release 2019.10 milestone Aug 5, 2019
@miri64 miri64 requested a review from haukepetersen August 5, 2019 11:10
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 1, 2019
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 1, 2019

The same logic is used in the else case already, so this change is consistent with the rest of the implementation.

However, both contradict the MQTT documentation:

The Variable Byte Integer is encoded using an encoding scheme which uses a single byte for values up to 127. Larger values are handled as follows. The least significant seven bits of each byte encode the data, and the most significant bit is used to indicate whether there are bytes following in the representation. Thus, each byte encodes 128 values and a "continuation bit". The maximum number of bytes in the Variable Byte Integer field is four.

more examples

Apparently MQTT-SN != MQTT.

So yes, the code is correct.

Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Confirmed by reading the documentation.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 1, 2019

Apparently MQTT-SN != MQTT.

That explains, why I was confused by your mail. Let's give Murdock another spin at this before merging, just in case.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 1, 2019
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 1, 2019

Ahhh sorry, did not see that you triggered Murdock another hour ago. I thought the last run was ages past.

@miri64 miri64 merged commit aab312e into RIOT-OS:master Oct 1, 2019
@miri64 miri64 deleted the emcute/fix/length-calculation branch October 1, 2019 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants