From b28c4048f97811309c11f816650d5e2efd4ad6ce Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 20 Jun 2023 08:51:05 -0700 Subject: [PATCH] [fuzzing] Fix failures found by max_connection_idle_fuzzer (#33487) In chttp2: a pending but not yet sent goaway should block incoming requests just like a sent one (we will sent that data momentarily!) In the test: - handle the case of the connection idle timeout happening before the request arrives at the server - disable retries, as these cause the request to get stuck (as we don't have an additional server to retry on) Fix b/287897932 --------- Co-authored-by: ctiller --- .../chttp2/transport/chttp2_transport.cc | 3 +- .../ext/transport/chttp2/transport/parsing.cc | 4 +- ...ax_connection_idle_fuzzer-5373851475181568 | 601 ++++++++++++++++++ .../core/end2end/tests/max_connection_idle.cc | 56 +- 4 files changed, 644 insertions(+), 20 deletions(-) create mode 100644 test/core/end2end/end2end_test_corpus/max_connection_idle/clusterfuzz-testcase-minimized-max_connection_idle_fuzzer-5373851475181568 diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 7315c984a69..8bb037f2a98 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1818,8 +1818,9 @@ static void send_goaway(grpc_chttp2_transport* t, grpc_error_handle error, } else if (t->sent_goaway_state == GRPC_CHTTP2_NO_GOAWAY_SEND || t->sent_goaway_state == GRPC_CHTTP2_GRACEFUL_GOAWAY) { // We want to log this irrespective of whether http tracing is enabled - gpr_log(GPR_DEBUG, "%s: Sending goaway err=%s", + gpr_log(GPR_DEBUG, "%s %s: Sending goaway last_new_stream_id=%d err=%s", std::string(t->peer_string.as_string_view()).c_str(), + t->is_client ? "CLIENT" : "SERVER", t->last_new_stream_id, grpc_core::StatusToString(error).c_str()); t->sent_goaway_state = GRPC_CHTTP2_FINAL_GOAWAY_SEND_SCHEDULED; grpc_chttp2_goaway_append( diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index a6d73458669..689d4f7edd3 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -625,7 +625,9 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t, t->settings[GRPC_ACKED_SETTINGS] [GRPC_CHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS])) { return GRPC_ERROR_CREATE("Max stream count exceeded"); - } else if (t->sent_goaway_state == GRPC_CHTTP2_FINAL_GOAWAY_SENT) { + } else if (t->sent_goaway_state == GRPC_CHTTP2_FINAL_GOAWAY_SENT || + t->sent_goaway_state == + GRPC_CHTTP2_FINAL_GOAWAY_SEND_SCHEDULED) { GRPC_CHTTP2_IF_TRACING(gpr_log( GPR_INFO, "transport:%p SERVER peer:%s Final GOAWAY sent. Ignoring new " diff --git a/test/core/end2end/end2end_test_corpus/max_connection_idle/clusterfuzz-testcase-minimized-max_connection_idle_fuzzer-5373851475181568 b/test/core/end2end/end2end_test_corpus/max_connection_idle/clusterfuzz-testcase-minimized-max_connection_idle_fuzzer-5373851475181568 new file mode 100644 index 00000000000..f4722329496 --- /dev/null +++ b/test/core/end2end/end2end_test_corpus/max_connection_idle/clusterfuzz-testcase-minimized-max_connection_idle_fuzzer-5373851475181568 @@ -0,0 +1,601 @@ +event_engine_actions { + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 0 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 0 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + run_delay: 4143972351 + connections { + write_size: 0 + write_size: 126 + write_size: 1241513984 + write_size: 7299840 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 10 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 536870912 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 10 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 11993088 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 2785017857 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 126 + } + connections { + write_size: 0 + write_size: 126 + write_size: 7299840 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 1 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 536870912 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 128 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 3080192 + write_size: 126 + } + connections { + write_size: 2785017857 + } + connections { + write_size: 0 + write_size: 126 + write_size: 7299840 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 1 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 536870912 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 2785017857 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 128 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 0 + write_size: 3080192 + write_size: 126 + } +} diff --git a/test/core/end2end/tests/max_connection_idle.cc b/test/core/end2end/tests/max_connection_idle.cc index 7f68fed3979..fea7e0075d9 100644 --- a/test/core/end2end/tests/max_connection_idle.cc +++ b/test/core/end2end/tests/max_connection_idle.cc @@ -30,8 +30,8 @@ namespace grpc_core { namespace { -void SimpleRequestBody(CoreEnd2endTest& test) { - auto c = test.NewClientCall("/foo").Timeout(Duration::Seconds(30)).Create(); +bool SimpleRequestBody(CoreEnd2endTest& test) { + auto c = test.NewClientCall("/foo").Timeout(Duration::Minutes(1)).Create(); EXPECT_NE(c.GetPeer(), absl::nullopt); CoreEnd2endTest::IncomingMetadata server_initial_metadata; CoreEnd2endTest::IncomingStatusOnClient server_status; @@ -43,8 +43,25 @@ void SimpleRequestBody(CoreEnd2endTest& test) { .RecvInitialMetadata(server_initial_metadata) .RecvStatusOnClient(server_status); auto s = test.RequestCall(101); - test.Expect(101, true); + // Connection timeout may expire before we receive the request at the server, + // in which case we'll complete the client call but not the incoming call + // request from the server. + bool saw_request_at_server = false; + bool finished_client = false; + test.Expect(101, CoreEnd2endTest::Maybe{&saw_request_at_server}); + test.Expect(1, CoreEnd2endTest::Maybe{&finished_client}); test.Step(); + if (finished_client) { + EXPECT_FALSE(saw_request_at_server); + EXPECT_EQ(server_status.status(), GRPC_STATUS_UNAVAILABLE); + test.ShutdownServerAndNotify(1000); + test.Expect(1000, true); + test.Expect(101, false); + test.Step(); + return false; + } + EXPECT_FALSE(finished_client); + EXPECT_TRUE(saw_request_at_server); EXPECT_NE(s.GetPeer(), absl::nullopt); EXPECT_NE(c.GetPeer(), absl::nullopt); CoreEnd2endTest::IncomingCloseOnServer client_close; @@ -59,6 +76,7 @@ void SimpleRequestBody(CoreEnd2endTest& test) { EXPECT_EQ(server_status.message(), "xyz"); EXPECT_EQ(s.method(), "/foo"); EXPECT_FALSE(client_close.was_cancelled()); + return true; } CORE_END2END_TEST(RetryHttp2Test, MaxConnectionIdle) { @@ -69,8 +87,9 @@ CORE_END2END_TEST(RetryHttp2Test, MaxConnectionIdle) { .Set(GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS, Duration::Seconds(1).millis()) .Set(GRPC_ARG_MAX_RECONNECT_BACKOFF_MS, Duration::Seconds(1).millis()) - .Set(GRPC_ARG_MIN_RECONNECT_BACKOFF_MS, - Duration::Seconds(5).millis())); + .Set(GRPC_ARG_MIN_RECONNECT_BACKOFF_MS, Duration::Seconds(5).millis()) + // Avoid transparent retries for this test. + .Set(GRPC_ARG_ENABLE_RETRIES, false)); InitServer( ChannelArgs() .Set(GRPC_ARG_MAX_CONNECTION_IDLE_MS, kMaxConnectionIdle.millis()) @@ -90,19 +109,20 @@ CORE_END2END_TEST(RetryHttp2Test, MaxConnectionIdle) { GRPC_CHANNEL_TRANSIENT_FAILURE)); } // Use a simple request to cancel and reset the max idle timer - SimpleRequestBody(*this); - // wait for the channel to reach its maximum idle time - WatchConnectivityState(GRPC_CHANNEL_READY, - Duration::Seconds(3) + kMaxConnectionIdle, 99); - Expect(99, true); - Step(); - state = CheckConnectivityState(false); - EXPECT_THAT(state, - ::testing::AnyOf(GRPC_CHANNEL_IDLE, GRPC_CHANNEL_CONNECTING, - GRPC_CHANNEL_TRANSIENT_FAILURE)); - ShutdownServerAndNotify(1000); - Expect(1000, true); - Step(); + if (SimpleRequestBody(*this)) { + // wait for the channel to reach its maximum idle time + WatchConnectivityState(GRPC_CHANNEL_READY, + Duration::Seconds(3) + kMaxConnectionIdle, 99); + Expect(99, true); + Step(); + state = CheckConnectivityState(false); + EXPECT_THAT(state, + ::testing::AnyOf(GRPC_CHANNEL_IDLE, GRPC_CHANNEL_CONNECTING, + GRPC_CHANNEL_TRANSIENT_FAILURE)); + ShutdownServerAndNotify(1000); + Expect(1000, true); + Step(); + } } } // namespace