From 13acd20f7a862500f939fe422c0dc29f09007de3 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 12 Mar 2021 14:44:28 -0800 Subject: [PATCH] netty: Add Http2Headers.setLong() for inbound headers Starting in Netty 4.1.60, Netty will validate Content-Length headers using getAll() and setLong(). While getAll() was documented as only used in tests, it doesn't appear it was currently used in any tests. While Http2NettyTest.contentLengthPermitted() was added to confirm that Content-Length works, it won't actually exercise any interesting behavior until we upgrade to Netty 4.1.60. However, I did test with Netty 4.1.60 and it reproduced the failure in https://github.com/grpc/grpc-java/issues/7953 and passed with this change. Since Netty is now observing/modifying the headers, it would seem appropriate to implement a substantial portion of the Http2Headers API. However, the surface is much larger than we'd want to implement for a 'quick fix' that could be backported. In addition, it seems much of the API is just convenience methods, so it is probably appropriate to split out a AbstractHeaders class from DefaultHeaders in Netty that doesn't make any assumptions about the header storage mechanism. --- .../testing/integration/Http2NettyTest.java | 15 ++++ .../io/grpc/netty/GrpcHttp2HeadersUtils.java | 58 ++++++++++++++- .../grpc/netty/GrpcHttp2HeadersUtilsTest.java | 70 +++++++++++++++++-- 3 files changed, 135 insertions(+), 8 deletions(-) diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java index af4d6edabda..aead88e3135 100644 --- a/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java +++ b/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertNotEquals; import io.grpc.ChannelCredentials; +import io.grpc.Metadata; import io.grpc.ServerBuilder; import io.grpc.ServerCredentials; import io.grpc.TlsChannelCredentials; @@ -29,6 +30,7 @@ import io.grpc.netty.InternalNettyServerBuilder; import io.grpc.netty.NettyChannelBuilder; import io.grpc.netty.NettyServerBuilder; +import io.grpc.stub.MetadataUtils; import java.io.IOException; import java.net.InetAddress; import java.net.InetSocketAddress; @@ -101,4 +103,17 @@ public void localAddr() throws Exception { public void tlsInfo() { assertX500SubjectDn("CN=testclient, O=Internet Widgits Pty Ltd, ST=Some-State, C=AU"); } + + @Test + public void contentLengthPermitted() throws Exception { + // Some third-party gRPC implementations (e.g., ServiceTalk) include Content-Length. The HTTP/2 + // code starting in Netty 4.1.60.Final has special-cased handling of Content-Length, and may + // call uncommon methods on our custom headers implementation. + // https://github.com/grpc/grpc-java/issues/7953 + Metadata contentLength = new Metadata(); + contentLength.put(Metadata.Key.of("content-length", Metadata.ASCII_STRING_MARSHALLER), "5"); + blockingStub + .withInterceptors(MetadataUtils.newAttachHeadersInterceptor(contentLength)) + .emptyCall(EMPTY); + } } diff --git a/netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java b/netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java index 8f46c2c13dd..4bdef93ad04 100644 --- a/netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java +++ b/netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java @@ -39,7 +39,9 @@ import static io.netty.util.AsciiString.isUpperCase; import com.google.common.io.BaseEncoding; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import io.grpc.Metadata; +import io.netty.handler.codec.CharSequenceValueConverter; import io.netty.handler.codec.http2.DefaultHttp2HeadersDecoder; import io.netty.handler.codec.http2.Http2Headers; import io.netty.util.AsciiString; @@ -160,6 +162,44 @@ public List getAll(CharSequence csName) { return returnValues; } + @CanIgnoreReturnValue + @Override + public boolean remove(CharSequence csName) { + AsciiString name = requireAsciiString(csName); + int i = 0; + for (; i < namesAndValuesIdx; i += 2) { + if (equals(name, namesAndValues[i])) { + break; + } + } + if (i >= namesAndValuesIdx) { + return false; + } + int dest = i; + for (; i < namesAndValuesIdx; i += 2) { + if (equals(name, namesAndValues[i])) { + continue; + } + values[dest / 2] = values[i / 2]; + namesAndValues[dest] = namesAndValues[i]; + namesAndValues[dest + 1] = namesAndValues[i + 1]; + dest += 2; + } + namesAndValuesIdx = dest; + return true; + } + + @Override + public Http2Headers set(CharSequence name, CharSequence value) { + remove(name); + return add(name, value); + } + + @Override + public Http2Headers setLong(CharSequence name, long value) { + return set(name, AsciiString.of(CharSequenceValueConverter.INSTANCE.convertLong(value))); + } + /** * Returns the header names and values as bytes. An even numbered index contains the * {@code byte[]} representation of a header name (in insertion order), and the subsequent @@ -353,9 +393,6 @@ public CharSequence scheme() { return scheme; } - /** - * This method is called in tests only. - */ @Override public List getAll(CharSequence csName) { AsciiString name = requireAsciiString(csName); @@ -369,6 +406,21 @@ public List getAll(CharSequence csName) { return super.getAll(csName); } + @Override + public boolean remove(CharSequence csName) { + AsciiString name = requireAsciiString(csName); + if (isPseudoHeader(name)) { + // This code should never be reached. + throw new IllegalArgumentException("Use direct accessor methods for pseudo headers."); + } + if (equals(TE_HEADER, name)) { + boolean wasPresent = te != null; + te = null; + return wasPresent; + } + return super.remove(name); + } + /** * This method is called in tests only. */ diff --git a/netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java b/netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java index 9336035f897..11488b752f1 100644 --- a/netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java +++ b/netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java @@ -16,11 +16,10 @@ package io.grpc.netty; +import static com.google.common.truth.Truth.assertThat; import static io.grpc.Metadata.BINARY_BYTE_MARSHALLER; import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE; import static io.netty.util.AsciiString.of; -import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; @@ -131,7 +130,7 @@ public void decode_emptyHeaders() throws Http2Exception { Http2Headers decodedHeaders = decoder.decodeHeaders(3 /* randomly chosen */, encodedHeaders); assertEquals(0, decodedHeaders.size()); - assertThat(decodedHeaders.toString(), containsString("[]")); + assertThat(decodedHeaders.toString()).contains("[]"); } @Test @@ -156,8 +155,69 @@ public void dupBinHeadersWithComma() { values)); } + @Test + public void headerGetAll_notPresent() { + Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2); + http2Headers.add(AsciiString.of("notit"), AsciiString.of("val")); + assertThat(http2Headers.getAll(AsciiString.of("dont-care"))).isEmpty(); + } + + @Test + public void headerGetAll_multiplePresent() { + // getAll is used by Netty 4.1.60+. https://github.com/grpc/grpc-java/issues/7953 + Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2); + http2Headers.add(AsciiString.of("notit1"), AsciiString.of("val1")); + http2Headers.add(AsciiString.of("multiple"), AsciiString.of("value1")); + http2Headers.add(AsciiString.of("notit2"), AsciiString.of("val2")); + http2Headers.add(AsciiString.of("multiple"), AsciiString.of("value2")); + http2Headers.add(AsciiString.of("notit3"), AsciiString.of("val3")); + assertThat(http2Headers.size()).isEqualTo(5); + assertThat(http2Headers.getAll(AsciiString.of("multiple"))) + .containsExactly(AsciiString.of("value1"), AsciiString.of("value2")); + } + + @Test + public void headerRemove_notPresent() { + Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2); + http2Headers.add(AsciiString.of("dont-care"), AsciiString.of("value")); + assertThat(http2Headers.remove(AsciiString.of("not-seen"))).isFalse(); + assertThat(http2Headers.size()).isEqualTo(1); + assertThat(http2Headers.getAll(AsciiString.of("dont-care"))) + .containsExactly(AsciiString.of("value")); + } + + @Test + public void headerRemove_multiplePresent() { + Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2); + http2Headers.add(AsciiString.of("notit1"), AsciiString.of("val1")); + http2Headers.add(AsciiString.of("multiple"), AsciiString.of("value1")); + http2Headers.add(AsciiString.of("notit2"), AsciiString.of("val2")); + http2Headers.add(AsciiString.of("multiple"), AsciiString.of("value2")); + http2Headers.add(AsciiString.of("notit3"), AsciiString.of("val3")); + assertThat(http2Headers.remove(AsciiString.of("multiple"))).isTrue(); + assertThat(http2Headers.size()).isEqualTo(3); + assertThat(http2Headers.getAll(AsciiString.of("notit1"))) + .containsExactly(AsciiString.of("val1")); + assertThat(http2Headers.getAll(AsciiString.of("notit2"))) + .containsExactly(AsciiString.of("val2")); + assertThat(http2Headers.getAll(AsciiString.of("notit3"))) + .containsExactly(AsciiString.of("val3")); + } + + @Test + public void headerSetLong() { + // setLong is used by Netty 4.1.60+. https://github.com/grpc/grpc-java/issues/7953 + Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2); + http2Headers.add(AsciiString.of("long-header"), AsciiString.of("1")); + http2Headers.add(AsciiString.of("long-header"), AsciiString.of("2")); + http2Headers.setLong(AsciiString.of("long-header"), 3); + assertThat(http2Headers.size()).isEqualTo(1); + assertThat(http2Headers.getAll(AsciiString.of("long-header"))) + .containsExactly(AsciiString.of("3")); + } + private static void assertContainsKeyAndValue(String str, CharSequence key, CharSequence value) { - assertThat(str, containsString(key.toString())); - assertThat(str, containsString(value.toString())); + assertThat(str).contains(key.toString()); + assertThat(str).contains(value.toString()); } }