Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions include/aws/http/private/h2_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@ struct aws_h2_stream {
* asleep. When stream needs to be awaken, moving the stream back to the outgoing_streams_list and set this bool
* to false */
bool waiting_for_writes;
/* Set when s_stream_complete has been called. Prevents double-completion
* (e.g. GOAWAY completes stream, then pending cancel cross-thread task fires) */
bool is_complete;
} thread_data;

/* Any thread may touch this data, but the lock must be held (unless it's an atomic) */
Expand Down
7 changes: 7 additions & 0 deletions source/h2_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -1868,6 +1868,13 @@ static void s_handler_installed(struct aws_channel_handler *handler, struct aws_
static void s_stream_complete(struct aws_h2_connection *connection, struct aws_h2_stream *stream, int error_code) {
AWS_PRECONDITION(aws_channel_thread_is_callers_thread(connection->base.channel_slot->channel));

/* Guard against double-completion (e.g. GOAWAY completes stream, then pending cancel task fires) */
if (stream->thread_data.is_complete) {
AWS_H2_STREAM_LOG(DEBUG, stream, "stream already completed, ignoring.");
return;
}
stream->thread_data.is_complete = true;

/* Nice logging */
if (error_code) {
AWS_H2_STREAM_LOGF(
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,7 @@ add_net_test_case(h2_sm_mock_complete_stream)
add_net_test_case(h2_sm_mock_ideal_num_streams)
add_net_test_case(h2_sm_mock_large_ideal_num_streams)
add_net_test_case(h2_sm_mock_goaway)
add_net_test_case(h2_sm_mock_cancel_after_goaway_no_double_complete)
add_net_test_case(h2_sm_connection_ping)
add_net_test_case(h2_sm_with_flow_control_err)
add_net_test_case(h2_sm_with_initial_settings)
Expand Down
57 changes: 57 additions & 0 deletions tests/test_stream_manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -1152,6 +1152,63 @@ TEST_CASE(h2_sm_mock_goaway) {
return s_tester_clean_up();
}

/* Test that cancelling a stream after GOAWAY does not crash from double stream completion.
* Reproduces P428540711: cancel() schedules cross-thread task, but s_finish_shutdown already
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we mention this magic number?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's a comment in the test, so pretty much for ourselves to reference.

* completed the stream. The cross-thread task then invokes on_complete a second time. */
TEST_CASE(h2_sm_mock_cancel_after_goaway_no_double_complete) {
(void)ctx;
struct sm_tester_options options = {
.max_connections = 1,
.max_concurrent_streams_per_connection = 1,
.alloc = allocator,
};
ASSERT_SUCCESS(s_tester_init(&options));
s_override_cm_connect_function(s_aws_http_connection_manager_create_connection_sync_mock);

/* Acquire 1 stream */
ASSERT_SUCCESS(s_sm_stream_acquiring(1));
ASSERT_SUCCESS(s_wait_on_fake_connection_count(1));
s_drain_all_fake_connection_testing_channel();
ASSERT_SUCCESS(s_wait_on_streams_acquired_count(1));
ASSERT_INT_EQUALS(0, s_tester.acquiring_stream_errors);

struct sm_fake_connection *fake_connection = s_get_fake_connection(0);
ASSERT_SUCCESS(h2_fake_peer_send_connection_preface_default_settings(&fake_connection->peer));
testing_channel_drain_queued_tasks(&fake_connection->testing_channel);

/* Get the stream handle */
struct aws_http_stream *stream = NULL;
aws_array_list_front(&s_tester.streams, &stream);
ASSERT_NOT_NULL(stream);

/* Send GOAWAY with last_stream_id=0, meaning our stream (id=1) is rejected */
struct aws_byte_cursor debug_info;
AWS_ZERO_STRUCT(debug_info);
struct aws_h2_frame *goaway_frame =
aws_h2_frame_new_goaway(allocator, 0 /*last_stream_id*/, AWS_HTTP2_ERR_INTERNAL_ERROR, debug_info);
ASSERT_SUCCESS(h2_fake_peer_send_frame(&fake_connection->peer, goaway_frame));

/* Cancel the stream BEFORE draining tasks.
* This schedules the cross-thread work task (reset_called=true).
* When we drain, both the GOAWAY processing (which completes the stream)
* and the cancel cross-thread task will run, causing double on_complete. */
aws_http_stream_cancel_default_error(stream);

/* User releases their ref on the stream (simulates real usage: cancel then release).
* The stream must stay alive until the cross-thread task completes. */
aws_http_stream_release(stream);
aws_array_list_clear(&s_tester.streams);

/* Drain tasks - this should NOT crash from double stream completion or use-after-free */
testing_channel_drain_queued_tasks(&fake_connection->testing_channel);

/* Stream should be completed with error at least once */
ASSERT_TRUE(s_tester.stream_completed_count >= 1);
ASSERT_TRUE(s_tester.stream_complete_errors >= 1);

return s_tester_clean_up();
}

/* Test that PING works as expected. */
TEST_CASE(h2_sm_connection_ping) {
(void)ctx;
Expand Down
Loading