jbonofre commented on code in PR #1338:
URL: https://github.com/apache/arrow-java/pull/1338#discussion_r4421104470
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java:
##########
@@ -325,7 +373,10 @@ private static ArrowMessage frame(BufferAllocator
allocator, final InputStream s
break;
default:
- // ignore unknown fields.
+ // Anything else is an unknown field, including a known field
number sent with another
+ // wire type than expected. As in protobuf, it is skipped, not
rejected.
+ skipField(stream, tag);
Review Comment:
I believe a tag of `0x00` (field number 0) ends up here. Its wire type is
VARINT, so `skipField` consumes the next byte as its value.
protobuf rejects field number 0, so we could do the same at the top of
`skipField`:
```java
private static void skipField(InputStream stream, int tag, int depth) throws
IOException {
if (WireFormat.getTagFieldNumber(tag) == 0) {
throw new IOException("Malformed FlightData frame: invalid tag " + tag);
}
switch (WireFormat.getTagWireType(tag)) {
```
The `tag == 0` check in `skipGroup` can then be removed.
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java:
##########
@@ -77,6 +86,33 @@ class ArrowMessage implements AutoCloseable {
}
ENABLE_ZERO_COPY_READ = !"false".equalsIgnoreCase(zeroCopyReadFlag);
ENABLE_ZERO_COPY_WRITE = "true".equalsIgnoreCase(zeroCopyWriteFlag);
+
+ String maxGroupDepth =
System.getProperty("arrow.flight.max_protobuf_skip_group_depth");
+ if (maxGroupDepth == null) {
+ maxGroupDepth =
System.getenv("ARROW_FLIGHT_MAX_PROTOBUF_SKIP_GROUP_DEPTH");
+ }
+ MAX_PROTOBUF_SKIP_GROUP_DEPTH = parseMaxGroupDepth(maxGroupDepth);
+ }
+
+ private static final int DEFAULT_MAX_PROTOBUF_SKIP_GROUP_DEPTH = 100;
+
+ private static int parseMaxGroupDepth(String value) {
+ if (value == null) {
+ return DEFAULT_MAX_PROTOBUF_SKIP_GROUP_DEPTH;
+ }
+ try {
+ final int depth = Integer.parseInt(value.trim());
+ if (depth >= 0) {
Review Comment:
Maybe we should set an upper bound here, and skip group which is recursive
(`skipField` -> `skipGroup` -> `skipField`). With a large setting (like
`-Darrow.flight.max_protobuf_skip_group_depth=10000000`), a peer can send a
deeply nested message and overflow the stack.
`frame()` only catches `Exception`, so the `StackOverFlowError` skips the
cleanup: `appMetadata` and `body` are never released and the allocator leaks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
