diff --git a/include/aws/http/private/h2_stream.h b/include/aws/http/private/h2_stream.h index a6e3a8f07..0a97c5353 100644 --- a/include/aws/http/private/h2_stream.h +++ b/include/aws/http/private/h2_stream.h @@ -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) */ diff --git a/source/h2_connection.c b/source/h2_connection.c index 2c184546a..6bf9b7315 100644 --- a/source/h2_connection.c +++ b/source/h2_connection.c @@ -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( diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ee5e260e6..af57e205d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -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) diff --git a/tests/test_stream_manager.c b/tests/test_stream_manager.c index cc0134458..96ac5b4b0 100644 --- a/tests/test_stream_manager.c +++ b/tests/test_stream_manager.c @@ -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 + * 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;