-
Notifications
You must be signed in to change notification settings - Fork 30
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: coredump, dereference null variable #327
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test that triggers an error-code based completion?
Also check the other packet paths (unsubscribe, publish received) to see if they overlooked this too (probably).
Probably the simplest way of doing that is to set a low operation timeout and arrange the mqtt5 server mock to not response to subscribes at all. The operation will time out and there won't be a suback.
source/v5/mqtt5_to_mqtt3_adapter.c
Outdated
|
||
if (suback != NULL) { | ||
reason_code_count = suback->reason_code_count; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably move this down to the granted_qos
part, so we only need to do the check once.
if (suback == NULL) {
granted_qos = AWS_MQTT_QOS_FAILURE;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it twice so i don't change the scope of the variable granted_qos
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
==========================================
- Coverage 82.40% 82.26% -0.15%
==========================================
Files 20 20
Lines 8728 8732 +4
==========================================
- Hits 7192 7183 -9
- Misses 1536 1549 +13
☔ View full report in Codecov by Sentry. |
@@ -2686,6 +2686,88 @@ static int s_mqtt5to3_adapter_subscribe_single_success_fn(struct aws_allocator * | |||
|
|||
AWS_TEST_CASE(mqtt5to3_adapter_subscribe_single_success, s_mqtt5to3_adapter_subscribe_single_success_fn) | |||
|
|||
/* | |||
* This function tests receiving a subscribe acknowlege after disconnecting from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo acknowlege -> acknowledge
source/v5/mqtt5_to_mqtt3_adapter.c
Outdated
struct aws_mqtt_topic_subscription *subscription_ptr = | ||
(struct aws_mqtt_topic_subscription *)multi_sub_subscription_buf; | ||
|
||
struct aws_array_list multi_sub_list; | ||
aws_array_list_init_static( | ||
&multi_sub_list, | ||
multi_sub_subscription_ptr_buf, | ||
suback->reason_code_count, | ||
reason_code_count, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aws_array_list_init_static
has a check for the third parameter:
AWS_FATAL_PRECONDITION(item_count > 0);
It seems we should handle reason_code_count == 0
separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like a null suback may be normal and expected behavior. I don't think we want to fail the AWS_FATAL_PRECONDITION
on something we're doing intentionally (e.g. op timeout, close connection). We may want to set the subscription_count
earlier and use it since it's grabbing that from the stored &subscribe_op->subscriptions)
array length on L2250.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is an expected behavior, this function is called with an explicit null as the suback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this case, we would need a test for suback on multiple subscription as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, maybe we need a test that sets the multi suback but not the singular one so it hits the multi processing.
@@ -2207,7 +2215,7 @@ static void s_aws_mqtt5_to_mqtt3_adapter_subscribe_completion_fn( | |||
topic_filter = record->subscription_view.topic_filter; | |||
} | |||
|
|||
if (suback->reason_code_count > 0) { | |||
if (reason_code_count > 0) { | |||
granted_qos = s_convert_mqtt5_suback_reason_code_to_mqtt3_granted_qos(suback->reason_codes[0]); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trivial: Instead of checking on L2206, could we add an else block and assign AWS_MQTT_QOS_FAILURE
to granted_qos
here to simplify where that is getting assigned to one place? It'd also remove the second suback == NULL
check that Vera mentioned.
source/v5/mqtt5_to_mqtt3_adapter.c
Outdated
@@ -2226,23 +2234,22 @@ static void s_aws_mqtt5_to_mqtt3_adapter_subscribe_completion_fn( | |||
"id=%p: mqtt3-to-5-adapter, completing multi-topic subscribe", | |||
(void *)adapter); | |||
|
|||
AWS_VARIABLE_LENGTH_ARRAY(struct aws_mqtt_topic_subscription, multi_sub_subscription_buf, reason_code_count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With reason_code_count
equal to 0, the expression here will be
struct aws_mqtt_topic_subscription multi_sub_subscription_buf[0];
, which is invalid.
It's just another reason to handle reason_code_count == 0
separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we have a reason code of zero without suback being null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the docs, the number of reason codes must match the number of topics. Though, I'd say a simple check probably won't hurt.
source/v5/mqtt5_to_mqtt3_adapter.c
Outdated
sizeof(struct aws_mqtt_topic_subscription *)); | ||
|
||
size_t subscription_count = aws_array_list_length(&subscribe_op->subscriptions); | ||
|
||
for (size_t i = 0; i < suback->reason_code_count; ++i) { | ||
for (size_t i = 0; i < reason_code_count; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here as above. Use subscription_count to process and report back the failed sub for all subscriptions. We will also need to assign the subscription->qos
to the correct response.
source/v5/mqtt5_to_mqtt3_adapter.c
Outdated
sizeof(struct aws_mqtt_topic_subscription *)); | ||
|
||
size_t subscription_count = aws_array_list_length(&subscribe_op->subscriptions); | ||
|
||
for (size_t i = 0; i < suback->reason_code_count; ++i) { | ||
for (size_t i = 0; i < reason_code_count; ++i) { | ||
struct aws_mqtt_topic_subscription *subscription = subscription_ptr + i; | ||
AWS_ZERO_STRUCT(*subscription); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check reason_code_count
or suback == NULL
so we don't try to access suback->reason_codes
if it's NULL and set the subscription->qos
toAWS_MQTT_QOS_FAILURE
source/v5/mqtt5_to_mqtt3_adapter.c
Outdated
"id=%p: mqtt3-to-5-adapter, completing multi-topic subscribe", | ||
(void *)adapter); | ||
(*subscribe_op->on_multi_suback)( | ||
&adapter->base, subscribe_op->base.id, NULL, error_code, subscribe_op->on_multi_suback_user_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aws-crt-cpp
expects the list to be non-empty. It won't crash, but it'll set the qos to AWS_MQTT_QOS_AT_MOST_ONCE
and then pass empty list of topics to the user callback. Which may be confusing.
Here's the corresponding code: https://github.com/awslabs/aws-crt-cpp/blob/main/source/mqtt/MqttConnectionCore.cpp#L414
Probably the empty list should be handled in aws-crt-cpp
. But I feel like it should be done here, so all the layers above don't have to worry about these things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it. I think it might be better to handle it on CRT side, as it might be better to check NULL value for a pointer regardless.
I will update the changes in CRTs.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.