From e866b00bc9da7bd59e372f7717f3eed33a57f90d Mon Sep 17 00:00:00 2001 From: Jacob Pratt Date: Mon, 19 Aug 2024 05:49:33 -0400 Subject: [PATCH 1/5] Remove unnecessary trait bounds from server (#1872) --- tonic-build/src/server.rs | 16 ++++++---------- tonic-health/src/generated/grpc_health_v1.rs | 8 ++++---- .../src/generated/grpc_reflection_v1.rs | 8 ++++---- .../src/generated/grpc_reflection_v1alpha.rs | 8 ++++---- 4 files changed, 18 insertions(+), 22 deletions(-) diff --git a/tonic-build/src/server.rs b/tonic-build/src/server.rs index 733b3ff89..c633f10bb 100644 --- a/tonic-build/src/server.rs +++ b/tonic-build/src/server.rs @@ -52,7 +52,7 @@ pub(crate) fn generate_internal( generate_doc_comments(service.comment()) }; - let named = generate_named(&server_service, &server_trait, &service_name); + let named = generate_named(&server_service, &service_name); let mod_attributes = attributes.for_mod(package); let struct_attributes = attributes.for_struct(&service_name); @@ -110,7 +110,7 @@ pub(crate) fn generate_internal( #service_doc #(#struct_attributes)* #[derive(Debug)] - pub struct #server_service { + pub struct #server_service { inner: Arc, accept_compression_encodings: EnabledCompressionEncodings, send_compression_encodings: EnabledCompressionEncodings, @@ -118,7 +118,7 @@ pub(crate) fn generate_internal( max_encoding_message_size: Option, } - impl #server_service { + impl #server_service { pub fn new(inner: T) -> Self { Self::from_arc(Arc::new(inner)) } @@ -175,7 +175,7 @@ pub(crate) fn generate_internal( } } - impl Clone for #server_service { + impl Clone for #server_service { fn clone(&self) -> Self { let inner = self.inner.clone(); Self { @@ -352,11 +352,7 @@ fn generate_trait_methods( stream } -fn generate_named( - server_service: &syn::Ident, - server_trait: &syn::Ident, - service_name: &str, -) -> TokenStream { +fn generate_named(server_service: &syn::Ident, service_name: &str) -> TokenStream { let service_name = syn::LitStr::new(service_name, proc_macro2::Span::call_site()); let name_doc = generate_doc_comment(" Generated gRPC service name"); @@ -364,7 +360,7 @@ fn generate_named( #name_doc pub const SERVICE_NAME: &'static str = #service_name; - impl tonic::server::NamedService for #server_service { + impl tonic::server::NamedService for #server_service { const NAME: &'static str = SERVICE_NAME; } } diff --git a/tonic-health/src/generated/grpc_health_v1.rs b/tonic-health/src/generated/grpc_health_v1.rs index 155300671..6f23598ef 100644 --- a/tonic-health/src/generated/grpc_health_v1.rs +++ b/tonic-health/src/generated/grpc_health_v1.rs @@ -243,14 +243,14 @@ pub mod health_server { ) -> std::result::Result, tonic::Status>; } #[derive(Debug)] - pub struct HealthServer { + pub struct HealthServer { inner: Arc, accept_compression_encodings: EnabledCompressionEncodings, send_compression_encodings: EnabledCompressionEncodings, max_decoding_message_size: Option, max_encoding_message_size: Option, } - impl HealthServer { + impl HealthServer { pub fn new(inner: T) -> Self { Self::from_arc(Arc::new(inner)) } @@ -427,7 +427,7 @@ pub mod health_server { } } } - impl Clone for HealthServer { + impl Clone for HealthServer { fn clone(&self) -> Self { let inner = self.inner.clone(); Self { @@ -441,7 +441,7 @@ pub mod health_server { } /// Generated gRPC service name pub const SERVICE_NAME: &'static str = "grpc.health.v1.Health"; - impl tonic::server::NamedService for HealthServer { + impl tonic::server::NamedService for HealthServer { const NAME: &'static str = SERVICE_NAME; } } diff --git a/tonic-reflection/src/generated/grpc_reflection_v1.rs b/tonic-reflection/src/generated/grpc_reflection_v1.rs index d76c6d899..0b5eb42f4 100644 --- a/tonic-reflection/src/generated/grpc_reflection_v1.rs +++ b/tonic-reflection/src/generated/grpc_reflection_v1.rs @@ -291,14 +291,14 @@ pub mod server_reflection_server { >; } #[derive(Debug)] - pub struct ServerReflectionServer { + pub struct ServerReflectionServer { inner: Arc, accept_compression_encodings: EnabledCompressionEncodings, send_compression_encodings: EnabledCompressionEncodings, max_decoding_message_size: Option, max_encoding_message_size: Option, } - impl ServerReflectionServer { + impl ServerReflectionServer { pub fn new(inner: T) -> Self { Self::from_arc(Arc::new(inner)) } @@ -436,7 +436,7 @@ pub mod server_reflection_server { } } } - impl Clone for ServerReflectionServer { + impl Clone for ServerReflectionServer { fn clone(&self) -> Self { let inner = self.inner.clone(); Self { @@ -450,7 +450,7 @@ pub mod server_reflection_server { } /// Generated gRPC service name pub const SERVICE_NAME: &'static str = "grpc.reflection.v1.ServerReflection"; - impl tonic::server::NamedService for ServerReflectionServer { + impl tonic::server::NamedService for ServerReflectionServer { const NAME: &'static str = SERVICE_NAME; } } diff --git a/tonic-reflection/src/generated/grpc_reflection_v1alpha.rs b/tonic-reflection/src/generated/grpc_reflection_v1alpha.rs index 1fc372d73..e6086c970 100644 --- a/tonic-reflection/src/generated/grpc_reflection_v1alpha.rs +++ b/tonic-reflection/src/generated/grpc_reflection_v1alpha.rs @@ -291,14 +291,14 @@ pub mod server_reflection_server { >; } #[derive(Debug)] - pub struct ServerReflectionServer { + pub struct ServerReflectionServer { inner: Arc, accept_compression_encodings: EnabledCompressionEncodings, send_compression_encodings: EnabledCompressionEncodings, max_decoding_message_size: Option, max_encoding_message_size: Option, } - impl ServerReflectionServer { + impl ServerReflectionServer { pub fn new(inner: T) -> Self { Self::from_arc(Arc::new(inner)) } @@ -436,7 +436,7 @@ pub mod server_reflection_server { } } } - impl Clone for ServerReflectionServer { + impl Clone for ServerReflectionServer { fn clone(&self) -> Self { let inner = self.inner.clone(); Self { @@ -450,7 +450,7 @@ pub mod server_reflection_server { } /// Generated gRPC service name pub const SERVICE_NAME: &'static str = "grpc.reflection.v1alpha.ServerReflection"; - impl tonic::server::NamedService for ServerReflectionServer { + impl tonic::server::NamedService for ServerReflectionServer { const NAME: &'static str = SERVICE_NAME; } } From c8754f3aaa267dc002302d182042e329193401dd Mon Sep 17 00:00:00 2001 From: bytetigers Date: Mon, 19 Aug 2024 22:34:51 +0800 Subject: [PATCH 2/5] chore: fix some comments (#1869) Signed-off-by: bytetigers --- tonic-types/src/richer_error/std_messages/retry_info.rs | 2 +- tonic/src/codec/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tonic-types/src/richer_error/std_messages/retry_info.rs b/tonic-types/src/richer_error/std_messages/retry_info.rs index a26e62076..93697afb1 100644 --- a/tonic-types/src/richer_error/std_messages/retry_info.rs +++ b/tonic-types/src/richer_error/std_messages/retry_info.rs @@ -16,7 +16,7 @@ use super::super::{pb, FromAny, IntoAny}; /// [error_details.proto]: https://github.com/googleapis/googleapis/blob/master/google/rpc/error_details.proto #[derive(Clone, Debug)] pub struct RetryInfo { - /// Informs the amout of time that clients should wait before retrying. + /// Informs the amount of time that clients should wait before retrying. pub retry_delay: Option, } diff --git a/tonic/src/codec/mod.rs b/tonic/src/codec/mod.rs index 0d33857c6..a5ac732ec 100644 --- a/tonic/src/codec/mod.rs +++ b/tonic/src/codec/mod.rs @@ -56,7 +56,7 @@ const DEFAULT_YIELD_THRESHOLD: usize = 32 * 1024; /// together into one larger send(). The yield threshold controls how /// much you want to bulk up such a batch of ready-to-send messages. /// The larger your yield threshold the more you will batch - and -/// consequentially allocate contiguous memory, which might be relevant +/// consequently allocate contiguous memory, which might be relevant /// if you're considering large numbers here. /// If your server streaming rpc does not reach the yield threshold /// before it reaches Poll::Pending (meaning, it's waiting for more From b4c351840ab2eaeb2734f194a40a755c4ca02424 Mon Sep 17 00:00:00 2001 From: grembo Date: Tue, 20 Aug 2024 11:32:29 +0200 Subject: [PATCH 3/5] Prevent server from exiting on ECONNABORTED (#1874) * Prevent server from exiting on ECONNABORTED On FreeBSD, accept can fail with ECONNABORTED, which means that "A connection arrived, but it was on the listen queue" (see `man 2 accept`). Without this patch, a server without the tls feature exits returing 0 in this case, which makes it vulnerable not only to intentional denial of service, but also to unintentional crashes, e.g., by haproxy TCP health checks. The problem can be reproduced on any FreeBSD system by running the tonic "helloworld" example (without feature TLS) and then sending a packet using nmap: cd examples cargo run --bin helloworld-server --no-default-features & nmap -sT -p 50051 -6 ::1 # server exits When running the example with the feature tls enabled, it won't exit (as the tls event loop in tonic/src/transport/server/incoming.rs handles errors gracefully): cd examples cargo run --bin helloworld-server --no-default-features \ features=tls & nmap -sT -p 50051 -6 ::1 # server keeps running This patch is not optimal - it removes some generic error parameters to gain access to `std::io::Error::kind()`. The logic itself should be sound. See also: - https://man.freebsd.org/cgi/man.cgi?accept(2) Accept man page - https://github.com/giampaolo/pyftpdlib/issues/105 https://github.com/giampaolo/pyftpdlib/commit/0f822329e3431ec1bc6250c03e938c65a61b2eb4 Basically the same issue (and its fix) in another project * Handle ECONNABORTED without breaking APIs This simplifies the previous patch a lot. The string search is ugly (also not 100% if it's needed or if we could handle errors like in the TLS enabled accept loop). * Next iteration based on feedback * More review feedback * Only use std::io if needed --- tonic/src/transport/server/incoming.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/tonic/src/transport/server/incoming.rs b/tonic/src/transport/server/incoming.rs index fe578b2a5..b7d2f8d6c 100644 --- a/tonic/src/transport/server/incoming.rs +++ b/tonic/src/transport/server/incoming.rs @@ -1,6 +1,8 @@ use super::service::ServerIo; #[cfg(feature = "tls")] use super::service::TlsAcceptor; +#[cfg(not(feature = "tls"))] +use std::io; use std::{ net::{SocketAddr, TcpListener as StdTcpListener}, pin::{pin, Pin}, @@ -27,7 +29,19 @@ where let mut incoming = pin!(incoming); while let Some(item) = incoming.next().await { - yield item.map(ServerIo::new_io)? + yield match item { + Ok(_) => item.map(ServerIo::new_io)?, + Err(e) => { + let e = e.into(); + tracing::debug!(error = %e, "accept loop error"); + if let Some(e) = e.downcast_ref::() { + if e.kind() == io::ErrorKind::ConnectionAborted { + continue; + } + } + Err(e)? + } + } } } } @@ -65,7 +79,7 @@ where } SelectOutput::Err(e) => { - tracing::debug!(message = "Accept loop error.", error = %e); + tracing::debug!(error = %e, "accept loop error"); } SelectOutput::Done => { From 7fb40a98f3279f8f11fc15752cd9cca7162f6972 Mon Sep 17 00:00:00 2001 From: tottoto Date: Tue, 20 Aug 2024 19:33:51 +0900 Subject: [PATCH 4/5] chore: Remove unnecessary static lifetime (#1877) --- tonic-build/src/server.rs | 2 +- tonic-health/src/generated/grpc_health_v1.rs | 2 +- tonic-reflection/src/generated/grpc_reflection_v1.rs | 2 +- tonic-reflection/src/generated/grpc_reflection_v1alpha.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tonic-build/src/server.rs b/tonic-build/src/server.rs index c633f10bb..670ec38b4 100644 --- a/tonic-build/src/server.rs +++ b/tonic-build/src/server.rs @@ -358,7 +358,7 @@ fn generate_named(server_service: &syn::Ident, service_name: &str) -> TokenStrea quote! { #name_doc - pub const SERVICE_NAME: &'static str = #service_name; + pub const SERVICE_NAME: &str = #service_name; impl tonic::server::NamedService for #server_service { const NAME: &'static str = SERVICE_NAME; diff --git a/tonic-health/src/generated/grpc_health_v1.rs b/tonic-health/src/generated/grpc_health_v1.rs index 6f23598ef..57c485466 100644 --- a/tonic-health/src/generated/grpc_health_v1.rs +++ b/tonic-health/src/generated/grpc_health_v1.rs @@ -440,7 +440,7 @@ pub mod health_server { } } /// Generated gRPC service name - pub const SERVICE_NAME: &'static str = "grpc.health.v1.Health"; + pub const SERVICE_NAME: &str = "grpc.health.v1.Health"; impl tonic::server::NamedService for HealthServer { const NAME: &'static str = SERVICE_NAME; } diff --git a/tonic-reflection/src/generated/grpc_reflection_v1.rs b/tonic-reflection/src/generated/grpc_reflection_v1.rs index 0b5eb42f4..6fc08092f 100644 --- a/tonic-reflection/src/generated/grpc_reflection_v1.rs +++ b/tonic-reflection/src/generated/grpc_reflection_v1.rs @@ -449,7 +449,7 @@ pub mod server_reflection_server { } } /// Generated gRPC service name - pub const SERVICE_NAME: &'static str = "grpc.reflection.v1.ServerReflection"; + pub const SERVICE_NAME: &str = "grpc.reflection.v1.ServerReflection"; impl tonic::server::NamedService for ServerReflectionServer { const NAME: &'static str = SERVICE_NAME; } diff --git a/tonic-reflection/src/generated/grpc_reflection_v1alpha.rs b/tonic-reflection/src/generated/grpc_reflection_v1alpha.rs index e6086c970..1fa73ac70 100644 --- a/tonic-reflection/src/generated/grpc_reflection_v1alpha.rs +++ b/tonic-reflection/src/generated/grpc_reflection_v1alpha.rs @@ -449,7 +449,7 @@ pub mod server_reflection_server { } } /// Generated gRPC service name - pub const SERVICE_NAME: &'static str = "grpc.reflection.v1alpha.ServerReflection"; + pub const SERVICE_NAME: &str = "grpc.reflection.v1alpha.ServerReflection"; impl tonic::server::NamedService for ServerReflectionServer { const NAME: &'static str = SERVICE_NAME; } From 086bcd2052ce4ddba11c7938ac2ac918d7da0b34 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Tue, 20 Aug 2024 18:43:38 +0800 Subject: [PATCH 5/5] server: allow setting max_header_list_size (#1870) * server: allow setting max_header_list_size Signed-off-by: Bugen Zhao * tweak style Signed-off-by: Bugen Zhao --------- Signed-off-by: Bugen Zhao --- tonic/src/transport/channel/endpoint.rs | 4 +++- tonic/src/transport/server/mod.rs | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/tonic/src/transport/channel/endpoint.rs b/tonic/src/transport/channel/endpoint.rs index 37ace4f12..508415f63 100644 --- a/tonic/src/transport/channel/endpoint.rs +++ b/tonic/src/transport/channel/endpoint.rs @@ -291,7 +291,9 @@ impl Endpoint { } } - /// Sets the max header list size. Uses `hyper`'s default otherwise. + /// Sets the max size of received header frames. + /// + /// This will default to whatever the default in hyper is. As of v1.4.1, it is 16 KiB. pub fn http2_max_header_list_size(self, size: u32) -> Self { Endpoint { http2_max_header_list_size: Some(size), diff --git a/tonic/src/transport/server/mod.rs b/tonic/src/transport/server/mod.rs index d2bfb0bbc..6cbe0bbe1 100644 --- a/tonic/src/transport/server/mod.rs +++ b/tonic/src/transport/server/mod.rs @@ -95,6 +95,7 @@ pub struct Server { http2_keepalive_timeout: Option, http2_adaptive_window: Option, http2_max_pending_accept_reset_streams: Option, + http2_max_header_list_size: Option, max_frame_size: Option, accept_http1: bool, service_builder: ServiceBuilder, @@ -117,6 +118,7 @@ impl Default for Server { http2_keepalive_timeout: None, http2_adaptive_window: None, http2_max_pending_accept_reset_streams: None, + http2_max_header_list_size: None, max_frame_size: None, accept_http1: false, service_builder: Default::default(), @@ -313,6 +315,17 @@ impl Server { } } + /// Sets the max size of received header frames. + /// + /// This will default to whatever the default in hyper is. As of v1.4.1, it is 16 KiB. + #[must_use] + pub fn http2_max_header_list_size(self, max: impl Into>) -> Self { + Server { + http2_max_header_list_size: max.into(), + ..self + } + } + /// Sets the maximum frame size to use for HTTP2. /// /// Passing `None` will do nothing. @@ -482,6 +495,7 @@ impl Server { http2_keepalive_timeout: self.http2_keepalive_timeout, http2_adaptive_window: self.http2_adaptive_window, http2_max_pending_accept_reset_streams: self.http2_max_pending_accept_reset_streams, + http2_max_header_list_size: self.http2_max_header_list_size, max_frame_size: self.max_frame_size, accept_http1: self.accept_http1, } @@ -514,6 +528,7 @@ impl Server { let init_stream_window_size = self.init_stream_window_size; let max_concurrent_streams = self.max_concurrent_streams; let timeout = self.timeout; + let max_header_list_size = self.http2_max_header_list_size; let max_frame_size = self.max_frame_size; let http2_only = !self.accept_http1; @@ -558,6 +573,10 @@ impl Server { .max_pending_accept_reset_streams(http2_max_pending_accept_reset_streams) .max_frame_size(max_frame_size); + if let Some(max_header_list_size) = max_header_list_size { + builder.http2().max_header_list_size(max_header_list_size); + } + builder };