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 websocket transfer callback userdata & server name in tls_options #313

Merged
merged 5 commits into from Aug 3, 2023
Merged

Fix websocket transfer callback userdata & server name in tls_options #313

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ddcb56
fix user data for websocket callbacks
xiazhvera Aug 2, 2023
ad30b2b
cheat with tls_options server name
xiazhvera Aug 3, 2023
f5380ee
check for tls option copy error
xiazhvera Aug 3, 2023
328eb06
update CR
xiazhvera Aug 3, 2023
2e4ee3c
update cr
xiazhvera Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 25 additions & 1 deletion source/v5/mqtt5_to_mqtt3_adapter.c
View file
Open in desktop
Expand Up @@ -314,6 +314,20 @@ static struct aws_mqtt_adapter_connect_task *s_aws_mqtt_adapter_connect_task_new
if (connection_options->tls_options) {
aws_tls_connection_options_copy(&connect_task->tls_options, connection_options->tls_options);
Copy link
Contributor

@sfod sfod Aug 3, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call can fail. Since s_aws_mqtt_adapter_connect_task_new is already changed, maybe add a check for this call as well?

connect_task->tls_options_ptr = &connect_task->tls_options;

/* Cheat and set the tls_options host_name to our copy if they're the same */
if (!connect_task->tls_options.server_name) {
struct aws_byte_cursor host_name_cur = aws_byte_cursor_from_buf(&connect_task->host_name);

if (aws_tls_connection_options_set_server_name(
&connect_task->tls_options, connect_task->allocator, &host_name_cur)) {
AWS_LOGF_ERROR(
AWS_LS_MQTT5_TO_MQTT3_ADAPTER,
"id=%p: mqtt3-to-5-adapter - Failed to set TLS Connection Options server name",
(void *)adapter);
goto error;
}
}
}

aws_byte_buf_init_copy_from_cursor(&connect_task->client_id, allocator, connection_options->client_id);
Expand All @@ -326,6 +340,13 @@ static struct aws_mqtt_adapter_connect_task *s_aws_mqtt_adapter_connect_task_new
connect_task->clean_session = connection_options->clean_session;

return connect_task;

error:
aws_ref_count_release(&adapter->internal_refs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, let's move the acquire after the last control flow that can error out. Having to release refs in error blocks is something to avoid if possible.

Copy link
Contributor Author

@xiazhvera xiazhvera Aug 3, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task is using the adapter->allocator. If we do not acquire the adapter beforehand, would there be chance that that the allocator get released with the adapter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no


s_aws_mqtt_adapter_connect_task_destroy(connect_task);

return NULL;
}

static int s_validate_adapter_connection_options(
Expand Down Expand Up @@ -1350,7 +1371,10 @@ static void s_aws_mqtt5_adapter_transform_websocket_handshake_fn(
adapter->mqtt5_websocket_handshake_completion_user_data = complete_ctx;

(*adapter->websocket_handshake_transformer)(
request, user_data, s_aws_mqtt5_adapter_websocket_handshake_completion_fn, adapter);
request,
adapter->websocket_handshake_transformer_user_data,
s_aws_mqtt5_adapter_websocket_handshake_completion_fn,
adapter);
} else {
(*complete_fn)(args.output_request, args.completion_error_code, complete_ctx);
}
Expand Down