Skip to content

Commit

Permalink
netty: Add Http2Headers.setLong() for inbound headers
Browse files Browse the repository at this point in the history 
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
grpc#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.
  • Loading branch information
@ejona86
ejona86 committed Mar 12, 2021
1 parent 14432df commit 1891de6
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 9 deletions.
15 changes: 15 additions & 0 deletions interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyTest.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}
58 changes: 55 additions & 3 deletions netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -160,6 +162,44 @@ public List<CharSequence> 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
Expand Down Expand Up @@ -353,9 +393,6 @@ public CharSequence scheme() {
return scheme;
}

/**
* This method is called in tests only.
*/
@Override
public List<CharSequence> getAll(CharSequence csName) {
AsciiString name = requireAsciiString(csName);
Expand All @@ -369,6 +406,21 @@ public List<CharSequence> 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.
*/
Expand Down
71 changes: 65 additions & 6 deletions netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -51,7 +50,6 @@
* Tests for {@link GrpcHttp2HeadersUtils}.
*/
@RunWith(JUnit4.class)
@SuppressWarnings({ "BadImport", "UndefinedEquals" }) // AsciiString.of and AsciiString.equals
public class GrpcHttp2HeadersUtilsTest {

private static final SensitivityDetector NEVER_SENSITIVE = new SensitivityDetector() {
Expand Down Expand Up @@ -131,7 +129,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
Expand All @@ -156,8 +154,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());
}
}

0 comments on commit 1891de6

Please sign in to comment.