- Every MQTT packet begins with a fixed header, whose second field is the
Remaining Length: the number of bytes that follow. It is not a byte. It
is spread over one to four bytes, seven value bits each, with the top
bit meaning "another byte follows". So values up to 127 take one byte,
and larger values take more.
Six packet types read it as a single quint8: SUBACK, UNSUBACK, PUBACK,
PUBREC, PUBREL and PUBCOMP. For most of them that used to be correct.
In MQTT 3.1.1 those packets contain nothing but a two byte identifier,
so the length was always exactly 2, which always fits in one byte. The
code did not even use the byte it read: it checked that the value was 2
and then hardcoded the length.
MQTT 5.0 support (ac66658) gave those packets an optional reason code
and properties, so the length can now be far above 127. That commit
switched off the "must be 2" check for MQTT 5.0 and began using the
byte as the real length, but kept reading only one byte. The check had
been the only thing making a one byte read safe, because a multi-byte
length always has its top bit set and so could never equal 2.
SUBACK never had a fixed length at all, since its payload carries one
reason code per subscribed topic filter. It has been misparsed for any
SUBACK above 127 bytes since the parser was written (d96011b).
Reading one byte of a two byte length consumes one byte too few, so the
remainder of the packet is then read as the next packet's header. The
client rejects a valid acknowledgement and stays out of step for the
rest of the connection. Brokers trigger this on ordinary traffic as
soon as they attach a reason string or user properties.
All six now use readVariableByteInteger(), and each rejects a length too
short to hold the fields the specification requires.
PUBREL also accepted a length of exactly 2 even for MQTT 5.0, which
rejected valid PUBREL packets carrying a reason code. It now accepts 2
or more there, and still requires exactly 2 for MQTT 3.1.1.
- Adds three regression tests driving the parser through the existing
FakeServer, so they need no broker.
Pick-to: 6.8
Task-number: QTBUG-149569
Change-Id: Ib239d836556e0cc14e13c116dee757fdd1a7ae81
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
(cherry picked from commit 06d301aa50)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit c662f644fd)
- finalize_pingresp() lowers m_pingTimeout every time, with no lower
limit. A broker can send a pile of unasked-for replies to push the
counter deep below zero, and then the client can spend some unexpected
amount of time to reach back the number that means:
"the broker has stopped answering, drop the link.”
- The fix prohibits pushing the counter below zero.
Task-number: QTBUG-149662
Pick-to: 6.8
Change-Id: I7b25e8ac3f83fa262a19010df9a91b2ee1c006fd
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
(cherry picked from commit 33538c5143)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 88430cff7a)
- finalize_suback() and finalize_unsuback() read the 2-byte request ID.
If no matching request was found, they returned immediately.
However, m_missingData remained non-zero.
The leftover bytes are then incorrectly interpreted as the header of
the next message!
- This change fixes the issue universally for all handlers in the
processDataHelper().
Pick-to: 6.8
Task-number: QTBUG-149570
Change-Id: I06c86414c3297caa386059a02fdec5c3765a0315
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
(cherry picked from commit 9e0428b917)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit eea36a1e4c)
- In the f8ecf4d56a we found that the
m_receiveAliases bounds check used a wrong property:
m_clientPrivate->m_serverConnectionProperties.maximumTopicAlias(),
but it must use instead:
m_clientPrivate->m_connectionProperties.maximumTopicAlias().
So, the idea is m_receiveAliases is sized according to the maximum
topic alias value declared by the client in CONNECT, while
m_publishAliases is sized according to the maximum value declared
by the server in CONNACK.
The previous fix did not update all related code.
1) The resize() calls for m_receiveAliases and m_publishAliases
still used the wrong properties.
2) The bounds check for m_publishAliases still used the client's
maximumTopicAlias() value instead of the server's.
- Update the resize() calls to use the appropriate connection
properties and fix the bounds check for m_publishAliases.
Amends f8ecf4d56a
Pick-to: 6.8
Task-number: QTBUG-149564
Change-Id: I252c67966bf28c6994f34b19024d1b7894748473
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
(cherry picked from commit 011eb1e424)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 86544d2644)
- Parsing continued after closeConnection(), leaving the client in the
"connected" state with a dead socket. This led to a permanent,
unrecoverable wedge in Release builds and a crash in Debug builds.
- Check whether the client is already disconnected before changing its
state to BrokerConnected.
Pick-to: 6.8
Task-number: QTBUG-149565
Change-Id: I13d45c24d24fe862d9aec6a7e421f5ef4d494db0
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
(cherry picked from commit c94175ff43)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit f5f164e7b3)
- On error, readVariableByteInteger() closes the connection and
returns-1. In this case, message handling should stop.
- Add checks for the return value to stop processing when -1 is returned.
Pick-to: 6.8
Task-number: QTBUG-149565
Change-Id: I8f6cc8d6f3011a89d3bd4033a3432f383e1e9164
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
(cherry picked from commit 13288574c1)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 514890bd67)
- The length reader checked its 4-byte limit after doing the maths.
So, a 5th byte was multiplied in and overflows before the value
was thrown away. But it was not noticed, because we thrown it away
and returned -1.
- The fix moves iteration++ and if (iteration > 4) before the
calculation, preventing the overflow.
Pick-to: 6.8
Task-number: QTBUG-149571
Change-Id: I78c2063c7e01af3e8f196f824216d31de92fab48
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
(cherry picked from commit ecaf42152e)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 1556fcfaef)
- The main differences between v2 and v3 are:
* `coin_module_test_template_v2.yaml` ignores autotest return
codes because the `COIN_CTEST_IGNORE_EXIT_CODE` flag is set.
* The Axivion build was moved from coin_module_build_template_v2.yaml
to coin_module_test_template_v3.yaml in commit
c3ddec2f15a1cf806d8df444952cb2c2231ae4ad
- Switch the qtmqtt submodule to the newer v3 version to improve
the submodule CI experience for autotests and restore Axivion
checks.
Change-Id: I1695c8ca1af616512dff437a0a4cdbd0491412d4
Reviewed-by: Toni Saario <toni.saario@qt.io>
(cherry picked from commit 96d602b65f)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit ad2e999485)
