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.

Sign up for GitHub

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

Jump to bottom

Fix: coredump, dereference null variable #327

Merged
merged 11 commits into from
Sep 21, 2023
Merged

Fix: coredump, dereference null variable #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
84c714a
Fix: coredump, dereference null variable
alfred2g Sep 18, 2023
17832d6
Format: clang-format
alfred2g Sep 18, 2023
d1a8040
Merge branch 'main' into fix_dereference
alfred2g Sep 18, 2023
fc11e14
Test: add testcase to test recption of subscribe ack after a disconnect
alfred2g Sep 20, 2023
d47bfd4
Format: Run clang-format
alfred2g Sep 20, 2023
db83c43
Resolve PR comments
alfred2g Sep 20, 2023
63bc364
Redesign
alfred2g Sep 20, 2023
312366e
Add logs
alfred2g Sep 20, 2023
b2baa5b
Refactor
alfred2g Sep 21, 2023
7003a9f
Fix duplicate statement
alfred2g Sep 21, 2023
039fcaa
Fix test, the mock server should not send suback for the test
xiazhvera Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 49 additions & 44 deletions source/v5/mqtt5_to_mqtt3_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2184,10 +2184,10 @@ static void s_aws_mqtt5_to_mqtt3_adapter_subscribe_completion_fn(
const struct aws_mqtt5_packet_suback_view *suback,
int error_code,
void *complete_ctx) {
(void)suback;

struct aws_mqtt5_to_mqtt3_adapter_operation_subscribe *subscribe_op = complete_ctx;
struct aws_mqtt_client_connection_5_impl *adapter = subscribe_op->base.adapter;
struct aws_mqtt_subscription_set_subscription_record *record = NULL;

if (subscribe_op->on_suback != NULL) {
AWS_LOGF_DEBUG(
Expand All @@ -2197,20 +2197,22 @@ static void s_aws_mqtt5_to_mqtt3_adapter_subscribe_completion_fn(

struct aws_byte_cursor topic_filter;
AWS_ZERO_STRUCT(topic_filter);

enum aws_mqtt_qos granted_qos = AWS_MQTT_QOS_AT_MOST_ONCE;

size_t subscription_count = aws_array_list_length(&subscribe_op->subscriptions);
if (subscription_count > 0) {
struct aws_mqtt_subscription_set_subscription_record *record = NULL;
aws_array_list_get_at(&subscribe_op->subscriptions, &record, 0);

topic_filter = record->subscription_view.topic_filter;
}

if (suback->reason_code_count > 0) {
granted_qos = s_convert_mqtt5_suback_reason_code_to_mqtt3_granted_qos(suback->reason_codes[0]);
if (suback != NULL) {
if (suback->reason_code_count > 0) {
granted_qos = s_convert_mqtt5_suback_reason_code_to_mqtt3_granted_qos(suback->reason_codes[0]);
}
} else {
granted_qos = AWS_MQTT_QOS_FAILURE;
}
Copy link
Contributor

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.


(*subscribe_op->on_suback)(
&adapter->base,
subscribe_op->base.id,
Expand All @@ -2226,47 +2228,50 @@ 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, suback->reason_code_count);
AWS_VARIABLE_LENGTH_ARRAY(
struct aws_mqtt_topic_subscription *, multi_sub_subscription_ptr_buf, suback->reason_code_count);
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,
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) {
struct aws_mqtt_topic_subscription *subscription = subscription_ptr + i;
AWS_ZERO_STRUCT(*subscription);

subscription->qos = s_convert_mqtt5_suback_reason_code_to_mqtt3_granted_qos(suback->reason_codes[i]);

if (i < subscription_count) {
struct aws_mqtt_subscription_set_subscription_record *record = NULL;
aws_array_list_get_at(&subscribe_op->subscriptions, &record, i);
if (suback == NULL) {
(*subscribe_op->on_multi_suback)(
&adapter->base, subscribe_op->base.id, NULL, error_code, subscribe_op->on_multi_suback_user_data);
} else {
AWS_VARIABLE_LENGTH_ARRAY(
struct aws_mqtt_topic_subscription, multi_sub_subscription_buf, suback->reason_code_count);
AWS_VARIABLE_LENGTH_ARRAY(
struct aws_mqtt_topic_subscription *, multi_sub_subscription_ptr_buf, suback->reason_code_count);
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,
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) {
struct aws_mqtt_topic_subscription *subscription = subscription_ptr + i;
AWS_ZERO_STRUCT(*subscription);

subscription->qos = s_convert_mqtt5_suback_reason_code_to_mqtt3_granted_qos(suback->reason_codes[i]);

if (i < subscription_count) {
aws_array_list_get_at(&subscribe_op->subscriptions, &record, i);

subscription->topic = record->subscription_view.topic_filter;
subscription->on_publish = record->subscription_view.on_publish_received;
subscription->on_publish_ud = record->subscription_view.callback_user_data;
subscription->on_cleanup = record->subscription_view.on_cleanup;
}

subscription->topic = record->subscription_view.topic_filter;
subscription->on_publish = record->subscription_view.on_publish_received;
subscription->on_publish_ud = record->subscription_view.callback_user_data;
subscription->on_cleanup = record->subscription_view.on_cleanup;
aws_array_list_push_back(&multi_sub_list, &subscription);
}

aws_array_list_push_back(&multi_sub_list, &subscription);
(*subscribe_op->on_multi_suback)(
&adapter->base,
subscribe_op->base.id,
&multi_sub_list,
error_code,
subscribe_op->on_multi_suback_user_data);
}

(*subscribe_op->on_multi_suback)(
&adapter->base,
subscribe_op->base.id,
&multi_sub_list,
error_code,
subscribe_op->on_multi_suback_user_data);
}

aws_mqtt5_to_mqtt3_adapter_operation_table_remove_operation(
Expand Down
2 changes: 2 additions & 0 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,8 @@ add_test_case(mqtt5to3_adapter_unsubscribe_overlapped)
add_test_case(mqtt5to3_adapter_get_stats)
add_test_case(mqtt5to3_adapter_resubscribe_nothing)
add_test_case(mqtt5to3_adapter_resubscribe_something)
add_test_case(mqtt5to3_adapter_subscribe_single_null_suback)
add_test_case(mqtt5to3_adapter_subscribe_multi_null_suback)

add_test_case(mqtt_subscription_set_add_empty_not_subbed)
add_test_case(mqtt_subscription_set_add_single_path)
Expand Down
161 changes: 161 additions & 0 deletions tests/v5/mqtt5_to_mqtt3_adapter_tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 acknowledge after disconnecting from
* the server.
* it expects a AWS_MQTT_QOS_FAILURE return
*/
static int s_mqtt5to3_adapter_subscribe_single_null_suback_fn(struct aws_allocator *allocator, void *ctx) {
(void)ctx;

aws_mqtt_library_init(allocator);

struct mqtt5_client_test_options test_options;
aws_mqtt5_client_test_init_default_options(&test_options);

struct aws_mqtt5_client_mqtt5_mock_test_fixture_options test_fixture_options = {
.client_options = &test_options.client_options,
.server_function_table = &test_options.server_function_table,
};

struct aws_mqtt5_to_mqtt3_adapter_test_fixture fixture;
ASSERT_SUCCESS(aws_mqtt5_to_mqtt3_adapter_test_fixture_init(&fixture, allocator, &test_fixture_options));

struct aws_mqtt_client_connection *connection = fixture.connection;

struct aws_mqtt_connection_options connection_options;
s_init_adapter_connection_options_from_fixture(&connection_options, &fixture);

connection_options.on_connection_complete = s_aws_mqtt5_to_mqtt3_adapter_test_fixture_record_connection_complete;
connection_options.user_data = &fixture;

aws_mqtt_client_connection_connect(connection, &connection_options);

s_wait_for_n_adapter_lifecycle_events(&fixture, AWS_MQTT3_LET_CONNECTION_COMPLETE, 1);

struct aws_byte_cursor topic = aws_byte_cursor_from_c_str("derp");

aws_mqtt_client_connection_subscribe(
connection,
&topic,
AWS_MQTT_QOS_AT_LEAST_ONCE,
s_aws_mqtt5_to_mqtt3_adapter_test_fixture_record_topic_specific_publish,
&fixture,
NULL,
s_aws_mqtt5_to_mqtt3_adapter_test_fixture_record_subscribe_complete,
&fixture);

struct aws_mqtt_topic_subscription expected_subs[1] = {
{
.topic = topic,
.qos = AWS_MQTT_QOS_FAILURE,
},
};

struct aws_mqtt3_operation_event expected_events[] = {
{
.type = AWS_MQTT3_OET_SUBSCRIBE_COMPLETE,
.error_code = AWS_ERROR_MQTT5_USER_REQUESTED_STOP,
},
};
aws_array_list_init_static_from_initialized(
&expected_events[0].granted_subscriptions,
(void *)expected_subs,
1,
sizeof(struct aws_mqtt_topic_subscription));

aws_mqtt_client_connection_disconnect(
connection, s_aws_mqtt5_to_mqtt3_adapter_test_fixture_record_disconnection_complete, &fixture);

s_wait_for_n_adapter_lifecycle_events(&fixture, AWS_MQTT3_LET_DISCONNECTION_COMPLETE, 1);

s_wait_for_n_adapter_operation_events(&fixture, AWS_MQTT3_OET_SUBSCRIBE_COMPLETE, 1);

ASSERT_SUCCESS(s_aws_mqtt5_to_mqtt3_adapter_test_fixture_verify_operation_sequence(
&fixture, AWS_ARRAY_SIZE(expected_events), expected_events, AWS_ARRAY_SIZE(expected_events)));

aws_mqtt5_to_mqtt3_adapter_test_fixture_clean_up(&fixture);
aws_mqtt_library_clean_up();

return AWS_OP_SUCCESS;
}

AWS_TEST_CASE(mqtt5to3_adapter_subscribe_single_null_suback, s_mqtt5to3_adapter_subscribe_single_null_suback_fn)

static void s_aws_mqtt5_to_mqtt3_adapter_test_fixture_record_subscribe_multi_complete(
struct aws_mqtt_client_connection *connection,
uint16_t packet_id,
Expand Down Expand Up @@ -2814,6 +2896,85 @@ static int s_mqtt5to3_adapter_subscribe_multi_success_fn(struct aws_allocator *a

AWS_TEST_CASE(mqtt5to3_adapter_subscribe_multi_success, s_mqtt5to3_adapter_subscribe_multi_success_fn)

/*
* This function tests receiving a subscribe acknowledge after disconnecting from
* the server.
* it expects a AWS_MQTT_QOS_FAILURE return
*/
static int s_mqtt5to3_adapter_subscribe_multi_null_suback_fn(struct aws_allocator *allocator, void *ctx) {
(void)ctx;

aws_mqtt_library_init(allocator);

struct mqtt5_client_test_options test_options;
aws_mqtt5_client_test_init_default_options(&test_options);

struct aws_mqtt5_client_mqtt5_mock_test_fixture_options test_fixture_options = {
.client_options = &test_options.client_options,
.server_function_table = &test_options.server_function_table,
};

struct aws_mqtt5_to_mqtt3_adapter_test_fixture fixture;
ASSERT_SUCCESS(aws_mqtt5_to_mqtt3_adapter_test_fixture_init(&fixture, allocator, &test_fixture_options));

struct aws_mqtt_client_connection *connection = fixture.connection;

struct aws_mqtt_connection_options connection_options;
s_init_adapter_connection_options_from_fixture(&connection_options, &fixture);

connection_options.on_connection_complete = s_aws_mqtt5_to_mqtt3_adapter_test_fixture_record_connection_complete;
connection_options.user_data = &fixture;

aws_mqtt_client_connection_connect(connection, &connection_options);

s_wait_for_n_adapter_lifecycle_events(&fixture, AWS_MQTT3_LET_CONNECTION_COMPLETE, 1);

struct aws_mqtt_topic_subscription subscriptions[] = {
{
.topic = aws_byte_cursor_from_c_str("topic/1"),
.qos = AWS_MQTT_QOS_AT_LEAST_ONCE,
},
{
.topic = aws_byte_cursor_from_c_str("topic/2"),
.qos = AWS_MQTT_QOS_AT_MOST_ONCE,
},
};

struct aws_array_list subscription_list;
aws_array_list_init_static_from_initialized(
&subscription_list, subscriptions, 2, sizeof(struct aws_mqtt_topic_subscription));

aws_mqtt_client_connection_subscribe_multiple(
connection,
&subscription_list,
s_aws_mqtt5_to_mqtt3_adapter_test_fixture_record_subscribe_multi_complete,
&fixture);

struct aws_mqtt3_operation_event expected_events[] = {
{
.type = AWS_MQTT3_OET_SUBSCRIBE_COMPLETE,
.error_code = AWS_ERROR_MQTT5_USER_REQUESTED_STOP,
},
};

aws_mqtt_client_connection_disconnect(
connection, s_aws_mqtt5_to_mqtt3_adapter_test_fixture_record_disconnection_complete, &fixture);

s_wait_for_n_adapter_lifecycle_events(&fixture, AWS_MQTT3_LET_DISCONNECTION_COMPLETE, 1);

s_wait_for_n_adapter_operation_events(&fixture, AWS_MQTT3_OET_SUBSCRIBE_COMPLETE, 1);

ASSERT_SUCCESS(s_aws_mqtt5_to_mqtt3_adapter_test_fixture_verify_operation_sequence(
&fixture, AWS_ARRAY_SIZE(expected_events), expected_events, AWS_ARRAY_SIZE(expected_events)));

aws_mqtt5_to_mqtt3_adapter_test_fixture_clean_up(&fixture);
aws_mqtt_library_clean_up();

return AWS_OP_SUCCESS;
}

AWS_TEST_CASE(mqtt5to3_adapter_subscribe_multi_null_suback, s_mqtt5to3_adapter_subscribe_multi_null_suback_fn)

static int s_mqtt5_mock_server_handle_subscribe_suback_failure(
void *packet,
struct aws_mqtt5_server_mock_connection_context *connection,
Expand Down