diff --git a/api/src/main/java/io/grpc/Metadata.java b/api/src/main/java/io/grpc/Metadata.java index fba2659776b..f277b9cd12a 100644 --- a/api/src/main/java/io/grpc/Metadata.java +++ b/api/src/main/java/io/grpc/Metadata.java @@ -94,15 +94,19 @@ public byte[] parseBytes(byte[] serialized) { * Simple metadata marshaller that encodes strings as is. * *

This should be used with ASCII strings that only contain the characters listed in the class - * comment of {@link AsciiMarshaller}. Otherwise the output may be considered invalid and - * discarded by the transport, or the call may fail. + * comment of {@link AsciiMarshaller}. Otherwise an {@link IllegalArgumentException} will be + * thrown. */ public static final AsciiMarshaller ASCII_STRING_MARSHALLER = new AsciiMarshaller() { @Override public String toAsciiString(String value) { - return value; + checkArgument( + value.chars().allMatch(c -> c >= 0x20 && c <= 0x7E), + "String \"%s\" contains non-printable ASCII characters", + value); + return value.trim(); } @Override diff --git a/api/src/test/java/io/grpc/MetadataTest.java b/api/src/test/java/io/grpc/MetadataTest.java index 14ba8ca9b23..ed139256c86 100644 --- a/api/src/test/java/io/grpc/MetadataTest.java +++ b/api/src/test/java/io/grpc/MetadataTest.java @@ -478,6 +478,17 @@ public void createFromPartial() { assertSame(anotherSalmon, h2.get(KEY_IMMUTABLE)); } + @Test + public void failNonPrintableAsciiCharacters() { + String value = "José"; + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("String \"" + value + "\" contains non-printable ASCII characters"); + + Metadata metadata = new Metadata(); + metadata.put(Metadata.Key.of("test-non-printable", Metadata.ASCII_STRING_MARSHALLER), value); + } + private static final class Fish { private String name; diff --git a/core/src/main/java/io/grpc/internal/TransportFrameUtil.java b/core/src/main/java/io/grpc/internal/TransportFrameUtil.java index f3c32416426..1ffd13898a5 100644 --- a/core/src/main/java/io/grpc/internal/TransportFrameUtil.java +++ b/core/src/main/java/io/grpc/internal/TransportFrameUtil.java @@ -24,7 +24,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import java.util.logging.Logger; import javax.annotation.CheckReturnValue; /** @@ -35,8 +34,6 @@ */ public final class TransportFrameUtil { - private static final Logger logger = Logger.getLogger(TransportFrameUtil.class.getName()); - private static final byte[] binaryHeaderSuffixBytes = Metadata.BINARY_HEADER_SUFFIX.getBytes(US_ASCII); @@ -57,26 +54,14 @@ public static byte[][] toHttp2Headers(Metadata headers) { for (int i = 0; i < serializedHeaders.length; i += 2) { byte[] key = serializedHeaders[i]; byte[] value = serializedHeaders[i + 1]; - if (endsWith(key, binaryHeaderSuffixBytes)) { - // Binary header. - serializedHeaders[k] = key; - serializedHeaders[k + 1] - = InternalMetadata.BASE64_ENCODING_OMIT_PADDING.encode(value).getBytes(US_ASCII); - k += 2; - } else { - // Non-binary header. - // Filter out headers that contain non-spec-compliant ASCII characters. - // TODO(zhangkun83): only do such check in development mode since it's expensive - if (isSpecCompliantAscii(value)) { - serializedHeaders[k] = key; - serializedHeaders[k + 1] = value; - k += 2; - } else { - String keyString = new String(key, US_ASCII); - logger.warning("Metadata key=" + keyString + ", value=" + Arrays.toString(value) - + " contains invalid ASCII characters"); - } - } + serializedHeaders[k] = key; + serializedHeaders[k + 1] = + endsWith(key, binaryHeaderSuffixBytes) + // Binary header. + ? InternalMetadata.BASE64_ENCODING_OMIT_PADDING.encode(value).getBytes(US_ASCII) + // Non-binary header. + : value; + k += 2; } // Fast path, everything worked out fine. if (k == serializedHeaders.length) { @@ -163,18 +148,5 @@ private static boolean endsWith(byte[] subject, byte[] suffix) { return true; } - /** - * Returns {@code true} if {@code subject} contains only bytes that are spec-compliant ASCII - * characters and space. - */ - private static boolean isSpecCompliantAscii(byte[] subject) { - for (byte b : subject) { - if (b < 32 || b > 126) { - return false; - } - } - return true; - } - private TransportFrameUtil() {} } diff --git a/core/src/test/java/io/grpc/internal/TransportFrameUtilTest.java b/core/src/test/java/io/grpc/internal/TransportFrameUtilTest.java index 8b4bc170d52..78b062c9a77 100644 --- a/core/src/test/java/io/grpc/internal/TransportFrameUtilTest.java +++ b/core/src/test/java/io/grpc/internal/TransportFrameUtilTest.java @@ -71,7 +71,6 @@ public void testToHttp2Headers() { Metadata headers = new Metadata(); headers.put(PLAIN_STRING, COMPLIANT_ASCII_STRING); headers.put(BINARY_STRING, NONCOMPLIANT_ASCII_STRING); - headers.put(BINARY_STRING_WITHOUT_SUFFIX, NONCOMPLIANT_ASCII_STRING); byte[][] http2Headers = TransportFrameUtil.toHttp2Headers(headers); // BINARY_STRING_WITHOUT_SUFFIX should not get in because it contains non-compliant ASCII // characters but doesn't have "-bin" in the name. @@ -96,7 +95,6 @@ public void testToAndFromHttp2Headers() { Metadata headers = new Metadata(); headers.put(PLAIN_STRING, COMPLIANT_ASCII_STRING); headers.put(BINARY_STRING, NONCOMPLIANT_ASCII_STRING); - headers.put(BINARY_STRING_WITHOUT_SUFFIX, NONCOMPLIANT_ASCII_STRING); byte[][] http2Headers = TransportFrameUtil.toHttp2Headers(headers); byte[][] rawSerialized = TransportFrameUtil.toRawSerializedHeaders(http2Headers); Metadata recoveredHeaders = InternalMetadata.newMetadata(rawSerialized);