From 765bfb1ce2b355e03717dce03f38f5821f995607 Mon Sep 17 00:00:00 2001 From: Kaloyan Date: Sat, 22 Apr 2023 12:20:27 +0300 Subject: [PATCH 1/5] protobuf,protobuf-lite: configurable protobuf recursion limit --- .../io/grpc/protobuf/lite/ProtoLiteUtils.java | 30 ++++- .../protobuf/lite/ProtoLiteUtilsTest.java | 118 ++++++++++++++++-- .../src/test/proto/simple_recursive.proto | 13 ++ .../java/io/grpc/protobuf/ProtoUtils.java | 12 +- 4 files changed, 153 insertions(+), 20 deletions(-) create mode 100644 protobuf-lite/src/test/proto/simple_recursive.proto diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index ddba5b8d5b1..b884e27aa63 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -81,7 +81,17 @@ public static void setExtensionRegistry(ExtensionRegistryLite newRegistry) { */ public static Marshaller marshaller(T defaultInstance) { // TODO(ejona): consider changing return type to PrototypeMarshaller (assuming ABI safe) - return new MessageMarshaller<>(defaultInstance); + return new MessageMarshaller<>(defaultInstance, -1); + } + + /** + * Creates a {@link Marshaller} for protos of the same type as {@code defaultInstance} and a + * custom limit for the recursion depth. Any negative number will leave the limit to its default + * value as defined by the protobuf library. + */ + public static Marshaller marshallerWithRecursionLimit( + T defaultInstance, int recursionLimit) { + return new MessageMarshaller<>(defaultInstance, recursionLimit); } /** @@ -94,7 +104,9 @@ public static Metadata.BinaryMarshaller metadataMarsh return new MetadataMarshaller<>(defaultInstance); } - /** Copies the data from input stream to output stream. */ + /** + * Copies the data from input stream to output stream. + */ static long copy(InputStream from, OutputStream to) throws IOException { // Copied from guava com.google.common.io.ByteStreams because its API is unstable (beta) checkNotNull(from, "inputStream cannot be null!"); @@ -117,18 +129,20 @@ private ProtoLiteUtils() { private static final class MessageMarshaller implements PrototypeMarshaller { + private static final ThreadLocal> bufs = new ThreadLocal<>(); private final Parser parser; private final T defaultInstance; + private final int recursionLimit; @SuppressWarnings("unchecked") - MessageMarshaller(T defaultInstance) { - this.defaultInstance = defaultInstance; - parser = (Parser) defaultInstance.getParserForType(); + MessageMarshaller(T defaultInstance, int recursionLimit) { + this.defaultInstance = checkNotNull(defaultInstance, "defaultInstance cannot be null"); + this.parser = (Parser) defaultInstance.getParserForType(); + this.recursionLimit = recursionLimit; } - @SuppressWarnings("unchecked") @Override public Class getMessageClass() { @@ -211,6 +225,10 @@ public T parse(InputStream stream) { // when parsing. cis.setSizeLimit(Integer.MAX_VALUE); + if (recursionLimit >= 0) { + cis.setRecursionLimit(recursionLimit); + } + try { return parseFrom(cis); } catch (InvalidProtocolBufferException ipbe) { diff --git a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java index 6ea836f96a7..d7a35956ca9 100644 --- a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java +++ b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; import com.google.common.io.ByteStreams; @@ -36,6 +37,8 @@ import io.grpc.Status; import io.grpc.StatusRuntimeException; import io.grpc.internal.GrpcUtil; +import io.grpc.testing.protobuf.SimpleRecursiveMessage; +import io.grpc.testing.protobuf.SimpleRecursiveMessage.Builder; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -47,14 +50,17 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Unit tests for {@link ProtoLiteUtils}. */ +/** + * Unit tests for {@link ProtoLiteUtils}. + */ @RunWith(JUnit4.class) public class ProtoLiteUtilsTest { @SuppressWarnings("deprecation") // https://github.com/grpc/grpc-java/issues/7467 - @Rule public final ExpectedException thrown = ExpectedException.none(); + @Rule + public final ExpectedException thrown = ExpectedException.none(); - private Marshaller marshaller = ProtoLiteUtils.marshaller(Type.getDefaultInstance()); + private final Marshaller marshaller = ProtoLiteUtils.marshaller(Type.getDefaultInstance()); private Type proto = Type.newBuilder().setName("name").build(); @Test @@ -85,8 +91,8 @@ public void testInvalidatedMessage() throws Exception { } @Test - public void parseInvalid() throws Exception { - InputStream is = new ByteArrayInputStream(new byte[] {-127}); + public void parseInvalid() { + InputStream is = new ByteArrayInputStream(new byte[]{-127}); try { marshaller.parse(is); fail("Expected exception"); @@ -97,7 +103,7 @@ public void parseInvalid() throws Exception { } @Test - public void testMismatch() throws Exception { + public void testMismatch() { Marshaller enumMarshaller = ProtoLiteUtils.marshaller(Enum.getDefaultInstance()); // Enum's name and Type's name are both strings with tag 1. Enum altProto = Enum.newBuilder().setName(proto.getName()).build(); @@ -105,7 +111,7 @@ public void testMismatch() throws Exception { } @Test - public void introspection() throws Exception { + public void introspection() { Marshaller enumMarshaller = ProtoLiteUtils.marshaller(Enum.getDefaultInstance()); PrototypeMarshaller prototypeMarshaller = (PrototypeMarshaller) enumMarshaller; assertSame(Enum.getDefaultInstance(), prototypeMarshaller.getMessagePrototype()); @@ -130,7 +136,8 @@ public void testAvailable() throws Exception { assertEquals(proto.getSerializedSize(), is.available()); is.read(); assertEquals(proto.getSerializedSize() - 1, is.available()); - while (is.read() != -1) {} + while (is.read() != -1) { + } assertEquals(-1, is.read()); assertEquals(0, is.available()); } @@ -203,7 +210,7 @@ public void metadataMarshaller_invalid() { Metadata.BinaryMarshaller metadataMarshaller = ProtoLiteUtils.metadataMarshaller(Type.getDefaultInstance()); try { - metadataMarshaller.parseBytes(new byte[] {-127}); + metadataMarshaller.parseBytes(new byte[]{-127}); fail("Expected exception"); } catch (IllegalArgumentException ex) { assertNotNull(((InvalidProtocolBufferException) ex.getCause()).getUnfinishedMessage()); @@ -219,7 +226,7 @@ public void extensionRegistry_notNull() { } @Test - public void parseFromKnowLengthInputStream() throws Exception { + public void parseFromKnowLengthInputStream() { Marshaller marshaller = ProtoLiteUtils.marshaller(Type.getDefaultInstance()); Type expect = Type.newBuilder().setName("expected name").build(); @@ -232,21 +239,106 @@ public void defaultMaxMessageSize() { assertEquals(GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE, ProtoLiteUtils.DEFAULT_MAX_MESSAGE_SIZE); } + @Test + public void testNullDefaultInstance() { + String expectedMessage = "defaultInstance cannot be null"; + assertThrows(expectedMessage, NullPointerException.class, + () -> ProtoLiteUtils.marshaller(null)); + + assertThrows(expectedMessage, NullPointerException.class, + () -> ProtoLiteUtils.marshallerWithRecursionLimit(null, 10) + ); + } + + @Test + public void givenPositiveLimit_testRecursionLimitExceeded() throws IOException { + Marshaller marshaller = ProtoLiteUtils.marshallerWithRecursionLimit( + SimpleRecursiveMessage.getDefaultInstance(), 10); + SimpleRecursiveMessage message = buildRecursiveMessage(12); + + assertRecursionLimitExceeded(marshaller, message); + } + + @Test + public void givenZeroLimit_testRecursionLimitExceeded() throws IOException { + Marshaller marshaller = ProtoLiteUtils.marshallerWithRecursionLimit( + SimpleRecursiveMessage.getDefaultInstance(), 0); + SimpleRecursiveMessage message = buildRecursiveMessage(1); + + assertRecursionLimitExceeded(marshaller, message); + } + + @Test + public void givenPositiveLimit_testRecursionLimitNotExceeded() throws IOException { + Marshaller marshaller = ProtoLiteUtils.marshallerWithRecursionLimit( + SimpleRecursiveMessage.getDefaultInstance(), 15); + SimpleRecursiveMessage message = buildRecursiveMessage(12); + + assertRecursionLimitNotExceeded(marshaller, message); + } + + @Test + public void givenZeroLimit_testRecursionLimitNotExceeded() throws IOException { + Marshaller marshaller = ProtoLiteUtils.marshallerWithRecursionLimit( + SimpleRecursiveMessage.getDefaultInstance(), 0); + SimpleRecursiveMessage message = buildRecursiveMessage(0); + + assertRecursionLimitNotExceeded(marshaller, message); + } + + @Test + public void testDefaultRecursionLimit() throws IOException { + Marshaller marshaller = ProtoLiteUtils.marshaller( + SimpleRecursiveMessage.getDefaultInstance()); + SimpleRecursiveMessage message = buildRecursiveMessage(100); + + assertRecursionLimitNotExceeded(marshaller, message); + } + + private static void assertRecursionLimitExceeded(Marshaller marshaller, + SimpleRecursiveMessage message) throws IOException { + InputStream is = marshaller.stream(message); + ByteArrayInputStream bais = new ByteArrayInputStream(ByteStreams.toByteArray(is)); + + assertThrows(StatusRuntimeException.class, () -> marshaller.parse(bais)); + } + + private static void assertRecursionLimitNotExceeded(Marshaller marshaller, + SimpleRecursiveMessage message) throws IOException { + InputStream is = marshaller.stream(message); + ByteArrayInputStream bais = new ByteArrayInputStream(ByteStreams.toByteArray(is)); + + assertEquals(message, marshaller.parse(bais)); + } + + private static SimpleRecursiveMessage buildRecursiveMessage(int depth) { + SimpleRecursiveMessage.Builder builder = SimpleRecursiveMessage.newBuilder() + .setValue("depth-" + depth); + for (int i = depth; i > 0; i--) { + builder = SimpleRecursiveMessage.newBuilder() + .setValue("depth-" + i) + .setMessage(builder.build()); + } + + return builder.build(); + } + private static class CustomKnownLengthInputStream extends InputStream implements KnownLength { + private int position = 0; - private byte[] source; + private final byte[] source; private CustomKnownLengthInputStream(byte[] source) { this.source = source; } @Override - public int available() throws IOException { + public int available() { return source.length - position; } @Override - public int read() throws IOException { + public int read() { if (position == source.length) { return -1; } diff --git a/protobuf-lite/src/test/proto/simple_recursive.proto b/protobuf-lite/src/test/proto/simple_recursive.proto new file mode 100644 index 00000000000..e3ff0f55634 --- /dev/null +++ b/protobuf-lite/src/test/proto/simple_recursive.proto @@ -0,0 +1,13 @@ +syntax = "proto3"; + +package grpc.testing; + +option java_package = "io.grpc.testing.protobuf"; +option java_outer_classname = "SimpleRecursiveProto"; +option java_multiple_files = true; + +// A simple recursive message for testing purposes +message SimpleRecursiveMessage { + string value = 1; + SimpleRecursiveMessage message = 2; +} diff --git a/protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java b/protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java index c936b3c1b48..ee21d9ef98b 100644 --- a/protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java +++ b/protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java @@ -57,6 +57,16 @@ public static Marshaller marshaller(final T defaultInstan return ProtoLiteUtils.marshaller(defaultInstance); } + /** + * Creates a {@link Marshaller} for protos of the same type as {@code defaultInstance} and a + * custom limit for the recursion depth. Any negative number will leave the limit to its default + * value as defined by the protobuf library. + */ + public static Marshaller marshallerWithRecursionLimit(T defaultInstance, + int recursionLimit) { + return ProtoLiteUtils.marshallerWithRecursionLimit(defaultInstance, recursionLimit); + } + /** * Produce a metadata key for a generated protobuf type. * @@ -70,7 +80,7 @@ public static Metadata.Key keyForProto(T instance) { /** * Produce a metadata marshaller for a protobuf type. - * + * * @since 1.13.0 */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/4477") From 324aa492d83f64704a6dea5cdf925e93b8504589 Mon Sep 17 00:00:00 2001 From: Kaloyan Date: Sun, 23 Apr 2023 11:38:18 +0300 Subject: [PATCH 2/5] remove unused import --- .../src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java index d7a35956ca9..6090c69d12b 100644 --- a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java +++ b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java @@ -38,7 +38,6 @@ import io.grpc.StatusRuntimeException; import io.grpc.internal.GrpcUtil; import io.grpc.testing.protobuf.SimpleRecursiveMessage; -import io.grpc.testing.protobuf.SimpleRecursiveMessage.Builder; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; From ab5469f2000a6a8fb37404d5fa4de32ba79a890b Mon Sep 17 00:00:00 2001 From: Kaloyan Date: Thu, 27 Apr 2023 23:43:57 +0300 Subject: [PATCH 3/5] add missing since and @ExperimentalApi --- .../src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java | 3 +++ protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java | 3 +++ 2 files changed, 6 insertions(+) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index b884e27aa63..9aa8bb036fe 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -88,7 +88,10 @@ public static Marshaller marshaller(T defaultInstance * Creates a {@link Marshaller} for protos of the same type as {@code defaultInstance} and a * custom limit for the recursion depth. Any negative number will leave the limit to its default * value as defined by the protobuf library. + * + * @since 1.56.0 */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10108") public static Marshaller marshallerWithRecursionLimit( T defaultInstance, int recursionLimit) { return new MessageMarshaller<>(defaultInstance, recursionLimit); diff --git a/protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java b/protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java index ee21d9ef98b..ebc708f522f 100644 --- a/protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java +++ b/protobuf/src/main/java/io/grpc/protobuf/ProtoUtils.java @@ -61,7 +61,10 @@ public static Marshaller marshaller(final T defaultInstan * Creates a {@link Marshaller} for protos of the same type as {@code defaultInstance} and a * custom limit for the recursion depth. Any negative number will leave the limit to its default * value as defined by the protobuf library. + * + * @since 1.56.0 */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10108") public static Marshaller marshallerWithRecursionLimit(T defaultInstance, int recursionLimit) { return ProtoLiteUtils.marshallerWithRecursionLimit(defaultInstance, recursionLimit); From aeb4797be85ed5e53b0284d4a7f436945e6df55a Mon Sep 17 00:00:00 2001 From: Kaloyan Simitchiyski Date: Tue, 2 May 2023 20:50:36 +0300 Subject: [PATCH 4/5] Fix formatting Co-authored-by: Sergii Tkachenko --- .../test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java index 6090c69d12b..c3075471e68 100644 --- a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java +++ b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java @@ -135,8 +135,7 @@ public void testAvailable() throws Exception { assertEquals(proto.getSerializedSize(), is.available()); is.read(); assertEquals(proto.getSerializedSize() - 1, is.available()); - while (is.read() != -1) { - } + while (is.read() != -1) {} assertEquals(-1, is.read()); assertEquals(0, is.available()); } From 47ab9aa724d8c025d337a226fc8c95f438146b4b Mon Sep 17 00:00:00 2001 From: Kaloyan Date: Tue, 2 May 2023 22:08:10 +0300 Subject: [PATCH 5/5] Revert unrelated formatting --- .../java/io/grpc/protobuf/lite/ProtoLiteUtils.java | 4 +--- .../io/grpc/protobuf/lite/ProtoLiteUtilsTest.java | 11 ++++------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java index 9aa8bb036fe..211ef5366e9 100644 --- a/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java +++ b/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java @@ -107,9 +107,7 @@ public static Metadata.BinaryMarshaller metadataMarsh return new MetadataMarshaller<>(defaultInstance); } - /** - * Copies the data from input stream to output stream. - */ + /** Copies the data from input stream to output stream. */ static long copy(InputStream from, OutputStream to) throws IOException { // Copied from guava com.google.common.io.ByteStreams because its API is unstable (beta) checkNotNull(from, "inputStream cannot be null!"); diff --git a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java index c3075471e68..5c25cb3b309 100644 --- a/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java +++ b/protobuf-lite/src/test/java/io/grpc/protobuf/lite/ProtoLiteUtilsTest.java @@ -49,15 +49,12 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Unit tests for {@link ProtoLiteUtils}. - */ +/** Unit tests for {@link ProtoLiteUtils}. */ @RunWith(JUnit4.class) public class ProtoLiteUtilsTest { @SuppressWarnings("deprecation") // https://github.com/grpc/grpc-java/issues/7467 - @Rule - public final ExpectedException thrown = ExpectedException.none(); + @Rule public final ExpectedException thrown = ExpectedException.none(); private final Marshaller marshaller = ProtoLiteUtils.marshaller(Type.getDefaultInstance()); private Type proto = Type.newBuilder().setName("name").build(); @@ -91,7 +88,7 @@ public void testInvalidatedMessage() throws Exception { @Test public void parseInvalid() { - InputStream is = new ByteArrayInputStream(new byte[]{-127}); + InputStream is = new ByteArrayInputStream(new byte[] {-127}); try { marshaller.parse(is); fail("Expected exception"); @@ -208,7 +205,7 @@ public void metadataMarshaller_invalid() { Metadata.BinaryMarshaller metadataMarshaller = ProtoLiteUtils.metadataMarshaller(Type.getDefaultInstance()); try { - metadataMarshaller.parseBytes(new byte[]{-127}); + metadataMarshaller.parseBytes(new byte[] {-127}); fail("Expected exception"); } catch (IllegalArgumentException ex) { assertNotNull(((InvalidProtocolBufferException) ex.getCause()).getUnfinishedMessage());