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

MQTT 5 Bug - Checks packet header but there is none (IDFGH-9603) #255

Closed
IvanSivius opened this issue Mar 9, 2023 · 1 comment
Closed

Comments

@IvanSivius
Copy link

Hi, I am currently working on a project that uses mqtt for the transmission of data from a monitoring system.
I found out that the library has a bug.
When you want to send a packet with a length that exceeds the maximum length of mqtt established, the Buffer is fragmented and sent in pieces.
In the funtion:

int esp_mqtt_client_publish(esp_mqtt_client_handle_t client, const char *topic, const char *data, int len, int qos, int retain)

you do:

   while (sending)  {
        if (mqtt_write_data(client) != ESP_OK) {}

This then goes in :

void esp_mqtt5_flow_control(esp_mqtt5_client_handle_t client){
  if (client->connect_info.protocol_ver == MQTT_PROTOCOL_V_5) {
      int msg_type = mqtt5_get_type(client->mqtt_state.outbound_message->data);
      if (msg_type == MQTT_MSG_TYPE_PUBLISH) {
          int msg_qos = mqtt5_get_qos(client->mqtt_state.outbound_message->data);
          if (msg_qos > 0) {
             client->send_publish_packet_count ++;
           ESP_LOGD(TAG, "Sent (%d) qos > 0 publish packet without ack", client->send_publish_packet_count)}}}}'

The first time you do this it is correct, you get the msg_type and msg_qos, but the second... we already entered the While (sending) in esp_mqtt_client_publish and then:

You copy the following buffer fragment in connection->buffer

Finally the bug:

         connection->message.data = connection->buffer;
        client->mqtt_state.outbound_message = &connection->message;

The problem here is that it then re-enters esp_mqtt5_flow_control but the header doesn't exist anymore because it's the next fragment with the payload only, causing the masks that want to get the msg_qos and the msg_type e to return wrong data.
This then increments the counter which then triggers a connectivity error as if you wanted to try to push things with Qos1 or Qos2 in esp_mqtt5_client_publish_check()

From my point of view esp_mqtt5_flow_control() should not be called again after the first fragment.

This is the first time I upload a bug, sorry if it was not the place. Thanks.

@github-actions github-actions bot changed the title MQTT 5 Bug - Checks packet header but there is none MQTT 5 Bug - Checks packet header but there is none (IDFGH-9603) Mar 9, 2023
@ESP-YJM
Copy link
Contributor

ESP-YJM commented Mar 17, 2023

@IvanSivius Thanks for reporting the issue. I will fix it as soon as possible.

espressif-bot pushed a commit to espressif/esp-idf that referenced this issue Sep 4, 2023
git log --oneline dffabb067fb3c39f486033d2e47eb4b1416f0c82..301bd9e0282eb993523b84d6c56d2559346aa071

Detailed description of the changes:
* fix: Added missing update to message data
  - See merge request espressif/esp-mqtt!189
  - See commit espressif/esp-mqtt@cc41d1b
* PR: fixing typos in `mqtt5_error_reason_code`
  - See merge request espressif/esp-mqtt!188
  - feat: Add enum definition with typo to keep backwards compatibility (espressif/esp-mqtt@90b4a45)
  - fixing typos in `mqtt5_error_reason_code` (espressif/esp-mqtt@dc775bb)
* docs: Clarify keepalive timeout
  - See merge request espressif/esp-mqtt!186
  - See commit espressif/esp-mqtt@cb1e6cf
* fix: LOG format strings
  - See merge request espressif/esp-mqtt!187
  - See commit espressif/esp-mqtt@a3b04f2
* Fix: Stop client only if it's running.
  - See merge request espressif/esp-mqtt!183
  - See commit espressif/esp-mqtt@36eec6f
* fix: Error on publish message creation was ignored.
  - See merge request espressif/esp-mqtt!185
  - See commit espressif/esp-mqtt@585e3ba
* Fix: Allocation for connection buffer was incorrectly done.
  - See merge request espressif/esp-mqtt!182
  - See commit espressif/esp-mqtt@6c849c6
* Adds mqtt host tests to Ci
  - See merge request espressif/esp-mqtt!181
  - See commit espressif/esp-mqtt@4050df4
* Fix: Outbox was leaked in case of initialization failure
  - See merge request espressif/esp-mqtt!180
  - See commit espressif/esp-mqtt@5d491a4
* feat: Add option to bind interface of use
  - See merge request espressif/esp-mqtt!179
  - Closes espressif/esp-mqtt#253
  - See commit espressif/esp-mqtt@363fbf7
* Add outbox size control feature
  - See merge request espressif/esp-mqtt!141
  - feat: Introduces outbox limit (espressif/esp-mqtt@372ab7b)
  - Removes unused outbox functions. (espressif/esp-mqtt@21a5491)
  - refactor: Group access to output buffer in mqtt_connection_t (espressif/esp-mqtt@122875b)
* Add custom transport configuration
  - See merge request espressif/esp-mqtt!169
  - feat: Add custom transport configuration (espressif/esp-mqtt@a5c1b44)
* Removes leftover calls to event_handler
  - See merge request espressif/esp-mqtt!178
  - See commit espressif/esp-mqtt@a492935
* PR: Added support to set server common name.
  - See merge request espressif/esp-mqtt!173
  - Added support to set server common name. (espressif/esp-mqtt@6195762)
* Merge branch 'bugfix/return_on_qos0_disconnected' into 'master'
  - See merge request espressif/esp-mqtt!175
  - See commit espressif/esp-mqtt@5bd9724
* Merge branch 'bugfix/cpp_compilation' into 'master'
  - See merge request espressif/esp-mqtt!174
  - See commit espressif/esp-mqtt@70cbaca
* bug: Incorrect return on disconnect qos0 publish
  - See merge request espressif/esp-mqtt!172
* Fix: Compilation in C++ with multiple subscribe
  - See merge request espressif/esp-mqtt!171
* ci: Fix qemu build against 5.1
  - See merge request espressif/esp-mqtt!170
  - See commit espressif/esp-mqtt@94defb8
* Minor cleanups on mqtt client
  - See merge request espressif/esp-mqtt!168
  - Removes pending message count (espressif/esp-mqtt@da6d38a)
  - Bugfix: Dispatch transport error on all write operations (espressif/esp-mqtt@5729048)
  - Merge enqueue functions (espressif/esp-mqtt@72833c7)
* Adds a configuration for outbox data destination
  - See merge request espressif/esp-mqtt!166
  - feat: Adds a configuration for outbox data destination (espressif/esp-mqtt@2c71f9e)
* CI: Add configuration for ttfw
  - See merge request espressif/esp-mqtt!167
  - See commit espressif/esp-mqtt@d71dcf3
* mqtt5: Fix flow control will increase count when send fragmented packet
  - See merge request espressif/esp-mqtt!164
  - Closes espressif/esp-mqtt#255
  - See commit espressif/esp-mqtt@5cce2c4
* Add license information to queue
  - See merge request espressif/esp-mqtt!163
  - See commit espressif/esp-mqtt@36f0faa
* MQTTv5: Fixes and additions from GitHub PRs
  - See merge request espressif/esp-mqtt!162
  - Merges espressif/esp-mqtt#250
  - fix: Allow MQTT v5 zero length payload (espressif/esp-mqtt@65a4fda)
  - feature: Include subscribe_id in esp_mqtt5_event_property_t (espressif/esp-mqtt@1011e63)
* Feature:  Enable SUBSCRIBE to multiple topics
  - See merge request espressif/esp-mqtt!156
  - See commit espressif/esp-mqtt@3210255
* Adds Kconfig option to configure poll read timeout
  - See merge request espressif/esp-mqtt!159
  - Closes: espressif/esp-mqtt#245
  - See commit espressif/esp-mqtt@2fa945d
* Fix formatting when using printf nano
  - See merge request espressif/esp-mqtt!160
  - See commit espressif/esp-mqtt@e9b865e
* mqtt5: Fix flow control will regard the DUP packet and not consider PUBCOMP packet
  - See merge request espressif/esp-mqtt!158
  - Closes espressif/esp-mqtt#243
  - See commit espressif/esp-mqtt@ed76036
* Remove possible null pointer dereferences
  - See merge request espressif/esp-mqtt!157
  - Bugfix: Remove Remove possible null pointer dereferences (espressif/esp-mqtt@f80772b)
espressif-bot pushed a commit to espressif/esp-idf that referenced this issue Sep 18, 2023
* Update submodule:
git log --oneline dffabb067fb3c39f486033d2e47eb4b1416f0c82..05b347643f6679cc60a50a9664b86a24ebf3ae10

Detailed description of the changes:
* Fix: Mock test should include idf_additions.h
  - See merge request espressif/esp-mqtt!191
  - fix: Mock tests include idf_additions.h (espressif/esp-mqtt@f35aaa1)
* Adds missing documentation to outbox configuration.
  - See merge request espressif/esp-mqtt!190
  - docs: Adds missing documentation to outbox configuration. (espressif/esp-mqtt@c355e0b)
* fix: Added missing update to message data
  - See merge request espressif/esp-mqtt!189
  - See commit espressif/esp-mqtt@cc41d1b
* PR: fixing typos in `mqtt5_error_reason_code`
  - See merge request espressif/esp-mqtt!188
  - feat: Add enum definition with typo to keep backwards compatibility (espressif/esp-mqtt@90b4a45)
  - fixing typos in `mqtt5_error_reason_code` (espressif/esp-mqtt@dc775bb)
* docs: Clarify keepalive timeout
  - See merge request espressif/esp-mqtt!186
  - See commit espressif/esp-mqtt@cb1e6cf
* fix: LOG format strings
  - See merge request espressif/esp-mqtt!187
  - See commit espressif/esp-mqtt@a3b04f2
* Fix: Stop client only if it's running.
  - See merge request espressif/esp-mqtt!183
  - See commit espressif/esp-mqtt@36eec6f
* fix: Error on publish message creation was ignored.
  - See merge request espressif/esp-mqtt!185
  - See commit espressif/esp-mqtt@585e3ba
* Fix: Allocation for connection buffer was incorrectly done.
  - See merge request espressif/esp-mqtt!182
  - See commit espressif/esp-mqtt@6c849c6
* Adds mqtt host tests to Ci
  - See merge request espressif/esp-mqtt!181
  - See commit espressif/esp-mqtt@4050df4
* Fix: Outbox was leaked in case of initialization failure
  - See merge request espressif/esp-mqtt!180
  - See commit espressif/esp-mqtt@5d491a4
* feat: Add option to bind interface of use
  - See merge request espressif/esp-mqtt!179
  - Closes espressif/esp-mqtt#253
  - See commit espressif/esp-mqtt@363fbf7
* Add outbox size control feature
  - See merge request espressif/esp-mqtt!141
  - feat: Introduces outbox limit (espressif/esp-mqtt@372ab7b)
  - Removes unused outbox functions. (espressif/esp-mqtt@21a5491)
  - refactor: Group access to output buffer in mqtt_connection_t (espressif/esp-mqtt@122875b)
* Add custom transport configuration
  - See merge request espressif/esp-mqtt!169
  - feat: Add custom transport configuration (espressif/esp-mqtt@a5c1b44)
* Removes leftover calls to event_handler
  - See merge request espressif/esp-mqtt!178
  - See commit espressif/esp-mqtt@a492935
* PR: Added support to set server common name.
  - See merge request espressif/esp-mqtt!173
  - Added support to set server common name. (espressif/esp-mqtt@6195762)
* Merge branch 'bugfix/return_on_qos0_disconnected' into 'master'
  - See merge request espressif/esp-mqtt!175
  - See commit espressif/esp-mqtt@5bd9724
* Merge branch 'bugfix/cpp_compilation' into 'master'
  - See merge request espressif/esp-mqtt!174
  - See commit espressif/esp-mqtt@70cbaca
* bug: Incorrect return on disconnect qos0 publish
  - See merge request espressif/esp-mqtt!172
* Fix: Compilation in C++ with multiple subscribe
  - See merge request espressif/esp-mqtt!171
* ci: Fix qemu build against 5.1
  - See merge request espressif/esp-mqtt!170
  - See commit espressif/esp-mqtt@94defb8
* Minor cleanups on mqtt client
  - See merge request espressif/esp-mqtt!168
  - Removes pending message count (espressif/esp-mqtt@da6d38a)
  - Bugfix: Dispatch transport error on all write operations (espressif/esp-mqtt@5729048)
  - Merge enqueue functions (espressif/esp-mqtt@72833c7)
* Adds a configuration for outbox data destination
  - See merge request espressif/esp-mqtt!166
  - feat: Adds a configuration for outbox data destination (espressif/esp-mqtt@2c71f9e)
* CI: Add configuration for ttfw
  - See merge request espressif/esp-mqtt!167
  - See commit espressif/esp-mqtt@d71dcf3
* mqtt5: Fix flow control will increase count when send fragmented packet
  - See merge request espressif/esp-mqtt!164
  - Closes espressif/esp-mqtt#255
  - See commit espressif/esp-mqtt@5cce2c4
* Add license information to queue
  - See merge request espressif/esp-mqtt!163
  - See commit espressif/esp-mqtt@36f0faa
* MQTTv5: Fixes and additions from GitHub PRs
  - See merge request espressif/esp-mqtt!162
  - Merges espressif/esp-mqtt#250
  - fix: Allow MQTT v5 zero length payload (espressif/esp-mqtt@65a4fda)
  - feature: Include subscribe_id in esp_mqtt5_event_property_t (espressif/esp-mqtt@1011e63)
* Feature:  Enable SUBSCRIBE to multiple topics
  - See merge request espressif/esp-mqtt!156
  - See commit espressif/esp-mqtt@3210255
* Adds Kconfig option to configure poll read timeout
  - See merge request espressif/esp-mqtt!159
  - Closes: espressif/esp-mqtt#245
  - See commit espressif/esp-mqtt@2fa945d
* Fix formatting when using printf nano
  - See merge request espressif/esp-mqtt!160
  - See commit espressif/esp-mqtt@e9b865e
* mqtt5: Fix flow control will regard the DUP packet and not consider PUBCOMP packet
  - See merge request espressif/esp-mqtt!158
  - Closes espressif/esp-mqtt#243
  - See commit espressif/esp-mqtt@ed76036
* Remove possible null pointer dereferences
  - See merge request espressif/esp-mqtt!157
  - Bugfix: Remove Remove possible null pointer dereferences (espressif/esp-mqtt@f80772b)
CommanderRedYT pushed a commit to CommanderRedYT/esp-idf that referenced this issue Sep 24, 2023
git log --oneline dffabb067fb3c39f486033d2e47eb4b1416f0c82..301bd9e0282eb993523b84d6c56d2559346aa071

Detailed description of the changes:
* fix: Added missing update to message data
  - See merge request espressif/esp-mqtt!189
  - See commit espressif/esp-mqtt@cc41d1b
* PR: fixing typos in `mqtt5_error_reason_code`
  - See merge request espressif/esp-mqtt!188
  - feat: Add enum definition with typo to keep backwards compatibility (espressif/esp-mqtt@90b4a45)
  - fixing typos in `mqtt5_error_reason_code` (espressif/esp-mqtt@dc775bb)
* docs: Clarify keepalive timeout
  - See merge request espressif/esp-mqtt!186
  - See commit espressif/esp-mqtt@cb1e6cf
* fix: LOG format strings
  - See merge request espressif/esp-mqtt!187
  - See commit espressif/esp-mqtt@a3b04f2
* Fix: Stop client only if it's running.
  - See merge request espressif/esp-mqtt!183
  - See commit espressif/esp-mqtt@36eec6f
* fix: Error on publish message creation was ignored.
  - See merge request espressif/esp-mqtt!185
  - See commit espressif/esp-mqtt@585e3ba
* Fix: Allocation for connection buffer was incorrectly done.
  - See merge request espressif/esp-mqtt!182
  - See commit espressif/esp-mqtt@6c849c6
* Adds mqtt host tests to Ci
  - See merge request espressif/esp-mqtt!181
  - See commit espressif/esp-mqtt@4050df4
* Fix: Outbox was leaked in case of initialization failure
  - See merge request espressif/esp-mqtt!180
  - See commit espressif/esp-mqtt@5d491a4
* feat: Add option to bind interface of use
  - See merge request espressif/esp-mqtt!179
  - Closes espressif/esp-mqtt#253
  - See commit espressif/esp-mqtt@363fbf7
* Add outbox size control feature
  - See merge request espressif/esp-mqtt!141
  - feat: Introduces outbox limit (espressif/esp-mqtt@372ab7b)
  - Removes unused outbox functions. (espressif/esp-mqtt@21a5491)
  - refactor: Group access to output buffer in mqtt_connection_t (espressif/esp-mqtt@122875b)
* Add custom transport configuration
  - See merge request espressif/esp-mqtt!169
  - feat: Add custom transport configuration (espressif/esp-mqtt@a5c1b44)
* Removes leftover calls to event_handler
  - See merge request espressif/esp-mqtt!178
  - See commit espressif/esp-mqtt@a492935
* PR: Added support to set server common name.
  - See merge request espressif/esp-mqtt!173
  - Added support to set server common name. (espressif/esp-mqtt@6195762)
* Merge branch 'bugfix/return_on_qos0_disconnected' into 'master'
  - See merge request espressif/esp-mqtt!175
  - See commit espressif/esp-mqtt@5bd9724
* Merge branch 'bugfix/cpp_compilation' into 'master'
  - See merge request espressif/esp-mqtt!174
  - See commit espressif/esp-mqtt@70cbaca
* bug: Incorrect return on disconnect qos0 publish
  - See merge request espressif/esp-mqtt!172
* Fix: Compilation in C++ with multiple subscribe
  - See merge request espressif/esp-mqtt!171
* ci: Fix qemu build against 5.1
  - See merge request espressif/esp-mqtt!170
  - See commit espressif/esp-mqtt@94defb8
* Minor cleanups on mqtt client
  - See merge request espressif/esp-mqtt!168
  - Removes pending message count (espressif/esp-mqtt@da6d38a)
  - Bugfix: Dispatch transport error on all write operations (espressif/esp-mqtt@5729048)
  - Merge enqueue functions (espressif/esp-mqtt@72833c7)
* Adds a configuration for outbox data destination
  - See merge request espressif/esp-mqtt!166
  - feat: Adds a configuration for outbox data destination (espressif/esp-mqtt@2c71f9e)
* CI: Add configuration for ttfw
  - See merge request espressif/esp-mqtt!167
  - See commit espressif/esp-mqtt@d71dcf3
* mqtt5: Fix flow control will increase count when send fragmented packet
  - See merge request espressif/esp-mqtt!164
  - Closes espressif/esp-mqtt#255
  - See commit espressif/esp-mqtt@5cce2c4
* Add license information to queue
  - See merge request espressif/esp-mqtt!163
  - See commit espressif/esp-mqtt@36f0faa
* MQTTv5: Fixes and additions from GitHub PRs
  - See merge request espressif/esp-mqtt!162
  - Merges espressif/esp-mqtt#250
  - fix: Allow MQTT v5 zero length payload (espressif/esp-mqtt@65a4fda)
  - feature: Include subscribe_id in esp_mqtt5_event_property_t (espressif/esp-mqtt@1011e63)
* Feature:  Enable SUBSCRIBE to multiple topics
  - See merge request espressif/esp-mqtt!156
  - See commit espressif/esp-mqtt@3210255
* Adds Kconfig option to configure poll read timeout
  - See merge request espressif/esp-mqtt!159
  - Closes: espressif/esp-mqtt#245
  - See commit espressif/esp-mqtt@2fa945d
* Fix formatting when using printf nano
  - See merge request espressif/esp-mqtt!160
  - See commit espressif/esp-mqtt@e9b865e
* mqtt5: Fix flow control will regard the DUP packet and not consider PUBCOMP packet
  - See merge request espressif/esp-mqtt!158
  - Closes espressif/esp-mqtt#243
  - See commit espressif/esp-mqtt@ed76036
* Remove possible null pointer dereferences
  - See merge request espressif/esp-mqtt!157
  - Bugfix: Remove Remove possible null pointer dereferences (espressif/esp-mqtt@f80772b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants