diff --git a/src/core/lib/transport/metadata_batch.cc b/src/core/lib/transport/metadata_batch.cc index 7dcd1a0dd57..9d992129ef6 100644 --- a/src/core/lib/transport/metadata_batch.cc +++ b/src/core/lib/transport/metadata_batch.cc @@ -64,7 +64,7 @@ absl::optional UnknownMap::GetStringValue( } // namespace metadata_detail ContentTypeMetadata::MementoType ContentTypeMetadata::ParseMemento( - Slice value, bool, MetadataParseErrorFn on_error) { + Slice value, bool, MetadataParseErrorFn /*on_error*/) { auto out = kInvalid; auto value_string = value.as_string_view(); if (value_string == "application/grpc") { @@ -76,7 +76,9 @@ ContentTypeMetadata::MementoType ContentTypeMetadata::ParseMemento( } else if (value_string.empty()) { out = kEmpty; } else { - on_error("invalid value", value); + // We are intentionally not invoking on_error here since the spec is not + // clear on what the behavior should be here, so to avoid breaking anyone, + // we should continue to accept this. } return out; } diff --git a/test/core/bad_client/tests/simple_request.cc b/test/core/bad_client/tests/simple_request.cc index 628b9d53f10..dcc650194fe 100644 --- a/test/core/bad_client/tests/simple_request.cc +++ b/test/core/bad_client/tests/simple_request.cc @@ -81,6 +81,27 @@ "\x10\x0cgrpc-timeout\x02" \ "5S" +#define PFX_STR_TEXT_HTML_CONTENT_TYPE_HEADER \ + "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" \ + "\x00\x00\x00\x04\x00\x00\x00\x00\x00" /* settings frame */ \ + "\x00\x00\xdf\x01\x04\x00\x00\x00\x01" /* headers: generated from \ + simple_request_text_html_content_type.headers \ + in this directory */ \ + "\x10\x05:path\x08/foo/bar" \ + "\x10\x07:scheme\x04http" \ + "\x10\x07:method\x04POST" \ + "\x10\x04host\x09localhost" \ + "\x10\x0c" \ + "content-type\x09text/html" \ + "\x10\x14grpc-accept-encoding\x15" \ + "deflate,identity,gzip" \ + "\x10\x02te\x08trailers" \ + "\x10\x0auser-agent\"bad-client grpc-c/0.12.0.0 (linux)" \ + "\x10\x0cgrpc-timeout\x03" \ + "10S" \ + "\x10\x0cgrpc-timeout\x02" \ + "5S" + static void verifier(grpc_server* server, grpc_completion_queue* cq, void* /*registered_method*/) { grpc_call_error error; @@ -107,6 +128,67 @@ static void verifier(grpc_server* server, grpc_completion_queue* cq, grpc_call_unref(s); } +static void VerifyRpcDoesNotGetCanceled(grpc_server* server, + grpc_completion_queue* cq, + void* /*registered_method*/) { + grpc_call_error error; + grpc_call* s; + grpc_call_details call_details; + grpc_core::CqVerifier cqv(cq); + grpc_metadata_array request_metadata_recv; + int was_cancelled = 2; + + grpc_call_details_init(&call_details); + grpc_metadata_array_init(&request_metadata_recv); + + error = grpc_server_request_call(server, &s, &call_details, + &request_metadata_recv, cq, cq, + grpc_core::CqVerifier::tag(101)); + GPR_ASSERT(GRPC_CALL_OK == error); + cqv.Expect(grpc_core::CqVerifier::tag(101), true); + cqv.Verify(); + + GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.host, "localhost")); + GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo/bar")); + + grpc_op* op; + grpc_op ops[6]; + // Send the initial metadata and the status from the server. + memset(ops, 0, sizeof(ops)); + op = ops; + op->op = GRPC_OP_SEND_INITIAL_METADATA; + op->data.send_initial_metadata.count = 0; + op->flags = 0; + op->reserved = nullptr; + op++; + op->op = GRPC_OP_RECV_CLOSE_ON_SERVER; + op->data.recv_close_on_server.cancelled = &was_cancelled; + op->flags = 0; + op->reserved = nullptr; + op++; + op->op = GRPC_OP_SEND_STATUS_FROM_SERVER; + op->data.send_status_from_server.trailing_metadata_count = 0; + op->data.send_status_from_server.status = GRPC_STATUS_UNIMPLEMENTED; + grpc_slice status_details = grpc_slice_from_static_string("xyz"); + op->data.send_status_from_server.status_details = &status_details; + op->flags = 0; + op->reserved = nullptr; + op++; + error = grpc_call_start_batch(s, ops, static_cast(op - ops), + grpc_core::CqVerifier::tag(103), nullptr); + GPR_ASSERT(GRPC_CALL_OK == error); + + cqv.Expect(grpc_core::CqVerifier::tag(103), true); + cqv.Verify(); + + // If the call had an error, `was_cancelled` would be 1. + // GPR_ASSERT(was_cancelled == 1); + + grpc_metadata_array_destroy(&request_metadata_recv); + grpc_call_details_destroy(&call_details); + grpc_call_unref(s); +} + static void failure_verifier(grpc_server* server, grpc_completion_queue* cq, void* /*registered_method*/) { while (grpc_core::Server::FromC(server)->HasOpenConnections()) { @@ -120,45 +202,50 @@ int main(int argc, char** argv) { grpc::testing::TestEnvironment env(&argc, argv); grpc_init(); - /* basic request: check that things are working */ + // basic request: check that things are working GRPC_RUN_BAD_CLIENT_TEST(verifier, nullptr, PFX_STR, 0); GRPC_RUN_BAD_CLIENT_TEST(verifier, nullptr, PFX_STR_UNUSUAL, 0); GRPC_RUN_BAD_CLIENT_TEST(verifier, nullptr, PFX_STR_UNUSUAL2, 0); - /* push an illegal data frame */ + // A basic request with a "content-type: text/html" header. The spec is + // not clear on what the behavior should be here, so to avoid breaking anyone, + // we should continue to accept this header. + GRPC_RUN_BAD_CLIENT_TEST(VerifyRpcDoesNotGetCanceled, nullptr, + PFX_STR_TEXT_HTML_CONTENT_TYPE_HEADER, 0); + + // push an illegal data frame GRPC_RUN_BAD_CLIENT_TEST(verifier, nullptr, PFX_STR "\x00\x00\x05\x00\x00\x00\x00\x00\x01" "\x34\x00\x00\x00\x00", 0); - /* push a data frame with bad flags */ + // push a data frame with bad flags GRPC_RUN_BAD_CLIENT_TEST(verifier, nullptr, PFX_STR "\x00\x00\x00\x00\x02\x00\x00\x00\x01", 0); - /* push a window update with a bad length */ + // push a window update with a bad length GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr, PFX_STR "\x00\x00\x01\x08\x00\x00\x00\x00\x01", 0); - /* push a window update with bad flags */ + // push a window update with bad flags GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr, PFX_STR "\x00\x00\x00\x08\x10\x00\x00\x00\x01", 0); - /* push a window update with bad data (0 is not legal window size increment) - */ + // push a window update with bad data (0 is not legal window size increment) GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr, PFX_STR "\x00\x00\x04\x08\x00\x00\x00\x00\x01" "\x00\x00\x00\x00", 0); - /* push a short goaway */ + // push a short goaway GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr, PFX_STR "\x00\x00\x04\x07\x00\x00\x00\x00\x00", 0); - /* disconnect before sending goaway */ + // disconnect before sending goaway GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr, PFX_STR "\x00\x01\x12\x07\x00\x00\x00\x00\x00", GRPC_BAD_CLIENT_DISCONNECT); - /* push a rst_stream with a bad length */ + // push a rst_stream with a bad length GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr, PFX_STR "\x00\x00\x01\x03\x00\x00\x00\x00\x01", 0); - /* push a rst_stream with bad flags */ + // push a rst_stream with bad flags GRPC_RUN_BAD_CLIENT_TEST(failure_verifier, nullptr, PFX_STR "\x00\x00\x00\x03\x10\x00\x00\x00\x01", 0); diff --git a/test/core/bad_client/tests/simple_request_text_html_content_type.headers b/test/core/bad_client/tests/simple_request_text_html_content_type.headers new file mode 100644 index 00000000000..4da77cb1246 --- /dev/null +++ b/test/core/bad_client/tests/simple_request_text_html_content_type.headers @@ -0,0 +1,13 @@ +# headers used in simple_request.c +# use tools/codegen/core/gen_header_frame.py to generate the binary strings +# contained in the source code +:path: /foo/bar +:scheme: http +:method: POST +host: localhost +content-type: text/html +grpc-accept-encoding: deflate,identity,gzip +te: trailers +user-agent: bad-client grpc-c/0.12.0.0 (linux) +grpc-timeout: 10S +grpc-timeout: 5S