Sync up on_suback_multi behavior between adapter and Mqtt3 client #328

xiazhvera · 2023-09-22T17:45:16Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sfod · 2023-09-22T23:04:35Z

source/v5/mqtt5_to_mqtt3_adapter.c


-                aws_array_list_push_back(&multi_sub_list, &subscription);
+            if (suback != NULL) {
+                subscription->qos = s_convert_mqtt5_suback_reason_code_to_mqtt3_granted_qos(suback->reason_codes[i]);


Debatable: If suback != null && length(suback->reason_codes) == 0, we'll get out of bounds access here.

It's debatable becuase when suback is not null then we received it from the server, and according to the mqtt standard the size of suback->reason_codes must be equal to the number of subscriptions. However, on the line 2258 we handle the case length(suback->reason_codes) > subscription_count, i.e. we handle the situation when the received suback packet violates the standard. So, for consistency, we should handle here the case suback != null && length(suback->reason_codes) == 0.
Or, maybe it'll be better to verify that length(suback->reason_codes) == subscription_count prior to this loop. And if it's not true, idk, at least add error log.

Debatable: I think instead of setting reason_code_count = subscription_count in the event of reason_code_count == 0, we should just use subscription_count in the creation of arrays and iterating in this loop. Then we could use if (i < reason_code_count) here which would account for both a null suback and length(suback->reason_codes) ==0 as well as being able to remove the if (i < subscription_count) check on 2258 since the loop is already doing that check.

Debatable: If suback != null && length(suback->reason_codes) == 0, we'll get out of bounds access here.

It's debatable becuase when suback is not null then we received it from the server, and according to the mqtt standard the size of suback->reason_codes must be equal to the number of subscriptions. However, on the line 2258 we handle the case length(suback->reason_codes) > subscription_count, i.e. we handle the situation when the received suback packet violates the standard. So, for consistency, we should handle here the case suback != null && length(suback->reason_codes) == 0. Or, maybe it'll be better to verify that length(suback->reason_codes) == subscription_count prior to this loop. And if it's not true, idk, at least add error log.

We don't need to worry about out of bounds here because the loop will loop through the reason_code_count as long as suback!=NULL. If suback->reason_codes) == 0, we will not go into the loop here.

sbSteveK · 2023-09-25T16:36:52Z

source/v5/mqtt5_to_mqtt3_adapter.c

-
-                if (i < subscription_count) {
-                    aws_array_list_get_at(&subscribe_op->subscriptions, &record, i);
+        // If the suback does not contains any data, we directly extract the data from subscribe_op


Trivial: "If suback does not contain data, we extract data from subscribe_op"

bretambrose · 2023-09-25T16:58:32Z

source/v5/mqtt5_to_mqtt3_adapter.c

-            }
+        if (reason_code_count > 0) {
+            granted_qos = s_convert_mqtt5_suback_reason_code_to_mqtt3_granted_qos(suback->reason_codes[0]);
+        } else if (record) {


I am confused as to what's going on here. Under no circumstances should we ever "invent" reason codes where none exist. A suback with no reason codes in it (or a mismatch relative to the subscribe) should tear down the connection as a protocol error. If we're not doing so atm, we should.

MQTT3 on a timeout will return a timeout error code with the original subscribe request topic and request qos. If we have no suback packet due to a timeout or shutting down the client in mqtt5, to emulate mqtt3 behavior we need to also send back the same along with the error code.

That's absurd

haha, yea. I had the same thought when I checked. The sins of our fathers...

Do any bindings do anything with those values when an error is set? There are also (buggy) cases where an error may not get set properly in which case doing this makes it look like the subscribe succeeded.

Yep, aws-crt-cpp extracts info regardless of the error code: https://github.com/awslabs/aws-crt-cpp/blob/main/source/mqtt/MqttConnectionCore.cpp#L417

Python bindings returns a future on a subscribe() and if there's an error code, sets an exception from the error_code in the future...

Yikes. So in cpp, it's up to the customer to check the error code to make sure they're not assuming successful subscribes. I need to double-check that we're not at least setting the request.qos to fail in a timeout. If we are, then we should be fine everywhere

Java binding will directly return error_code, while JS binding will extract the data regardless of the error_code.

It is probably reasonable to keep the topic and qos data when the subscription failed with an error_code. The user still wanna get the information of a failing subscription for possible operations. (For a simply example, the user would need the info to print out a proper error message, or try subscribe again on the same topic/qos value.)

codecov-commenter · 2023-09-27T18:08:28Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (7c467e4) 82.43% compared to head (8ac9a7d) 82.31%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
- Coverage   82.43%   82.31%   -0.13%     
==========================================
  Files          20       20              
  Lines        8732     8729       -3     
==========================================
- Hits         7198     7185      -13     
- Misses       1534     1544      +10

Files	Coverage Δ
source/v5/mqtt5_to_mqtt3_adapter.c	`76.09% <100.00%> (-0.06%)`	⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xiazhvera · 2023-09-28T17:40:46Z

I leave the on_suback behavior as it was in origin Mqtt311, where the client would return the subscription data even for a failure subscription. I think the user still wanna get the information of a failing subscription for possible operations. (For example, the user would need the info to print out a proper error message, or try subscribe again on the same topic/qos value)

bretambrose · 2023-09-28T18:26:52Z

source/v5/mqtt5_to_mqtt3_adapter.c

+        if (suback != NULL && suback->reason_code_count > 0) {
+            granted_qos = s_convert_mqtt5_suback_reason_code_to_mqtt3_granted_qos(suback->reason_codes[0]);
+        } else if (record) {
+            granted_qos = (enum aws_mqtt_qos)(record->subscription_view.qos);


Looking at this again, I think this is not what we want to do. This is a bug in the 311 client (to report the requested qos rather than the returned qos). We should fix the bug instead of making the 5 adapater also do the wrong thing.

This would apply to both the single and multi acks.

xiazhvera · 2024-01-09T22:17:35Z

Fixed mqtt3 behavior #340 instead of updating the adapter.

update multi_suback behavior

e610e20

xiazhvera changed the title ~~Update Behavior for Adapter on_suback_multi~~ Sync up on_suback_multi callback data for adapter and Mqtt3 client Sep 22, 2023

update subscribe single null suback test case

5f742a2

sfod reviewed Sep 22, 2023

View reviewed changes

sbSteveK reviewed Sep 25, 2023

View reviewed changes

bretambrose requested changes Sep 25, 2023

View reviewed changes

sfod requested review from alfred2g and removed request for alfred2g September 25, 2023 17:42

xiazhvera changed the title ~~Sync up on_suback_multi callback data for adapter and Mqtt3 client~~ Sync up on_suback_multi behavior between adapter and Mqtt3 client Sep 27, 2023

xiazhvera added 2 commits September 27, 2023 09:53

update the multi suback algorithm

908d5ea

revert suback error_code check

8ac9a7d

xiazhvera requested a review from bretambrose September 28, 2023 16:28

bretambrose reviewed Sep 28, 2023

View reviewed changes

xiazhvera closed this Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync up on_suback_multi behavior between adapter and Mqtt3 client #328

Sync up on_suback_multi behavior between adapter and Mqtt3 client #328

xiazhvera commented Sep 22, 2023

sfod Sep 22, 2023 •

edited

Loading

sbSteveK Sep 25, 2023

xiazhvera Sep 27, 2023

sbSteveK Sep 25, 2023 •

edited

Loading

bretambrose Sep 25, 2023

sbSteveK Sep 25, 2023

bretambrose Sep 25, 2023

sbSteveK Sep 25, 2023

bretambrose Sep 25, 2023 •

edited

Loading

sfod Sep 25, 2023

sbSteveK Sep 25, 2023

sbSteveK Sep 25, 2023

xiazhvera Sep 27, 2023

codecov-commenter commented Sep 27, 2023

xiazhvera commented Sep 28, 2023

bretambrose Sep 28, 2023

xiazhvera commented Jan 9, 2024

Sync up on_suback_multi behavior between adapter and Mqtt3 client #328

Sync up on_suback_multi behavior between adapter and Mqtt3 client #328

Conversation

xiazhvera commented Sep 22, 2023

sfod Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbSteveK Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bretambrose Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 27, 2023

Codecov Report

xiazhvera commented Sep 28, 2023

Choose a reason for hiding this comment

xiazhvera commented Jan 9, 2024

sfod Sep 22, 2023 •

edited

Loading

sbSteveK Sep 25, 2023 •

edited

Loading

bretambrose Sep 25, 2023 •

edited

Loading