nddipiazza commented on code in PR #3009:
URL: https://github.com/apache/tika/pull/3009#discussion_r3767328069
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/server/ServerProtocolIO.java:
##########
@@ -51,20 +53,75 @@ public class ServerProtocolIO {
private final DataInputStream input;
private final DataOutputStream output;
+ private final int maxPayloadBytes;
- public ServerProtocolIO(DataInputStream input, DataOutputStream output) {
+ public ServerProtocolIO(DataInputStream input, DataOutputStream output,
int maxPayloadBytes) {
this.input = input;
this.output = output;
+ this.maxPayloadBytes = maxPayloadBytes;
}
/**
* Writes a FINISHED message with the serialized result and waits for ACK.
+ * <p>
+ * Three-layer protection against oversized payloads:
+ * <ol>
+ * <li>Pre-check: if the estimated content size already exceeds the
limit, skip
+ * serialization entirely (prevents OOM for very large
documents).</li>
+ * <li>OOM catch: if serialization exhausts heap despite the pre-check
(e.g. when
+ * the limit is uncapped or the estimate is imprecise), the error is
caught and
+ * a lightweight PAYLOAD_LIMIT_EXCEEDED result is returned instead
of crashing.</li>
+ * <li>Post-check: if the serialized byte count exceeds the limit, the
oversized
+ * bytes are discarded before touching the wire, avoiding stream
desynchronization
+ * on the client side.</li>
+ * </ol>
*
* @throws ShutDownReceivedException if SHUT_DOWN is received instead of
ACK
* @throws IOException on serialization or I/O errors
*/
public void writeFinished(PipesResult pipesResult) throws IOException {
- byte[] bytes = JsonPipesIpc.toBytes(pipesResult);
+ // Pre-check: avoid allocating a huge byte[] when content is obviously
over-limit.
+ // estimateSizeInBytes() uses string.length() bytes (≈ UTF-8 Smile
bytes for ASCII),
+ // so this is a lower-bound estimate — safe to use as an early-exit
gate.
+ if (pipesResult.emitData() instanceof EmitDataImpl emitData) {
+ long estimated = emitData.getEstimatedSizeBytes();
+ if (estimated > maxPayloadBytes) {
+ LOG.warn("Skipping serialization: estimated payload {} bytes
exceeds maxIpcPayloadBytes {}",
+ estimated, maxPayloadBytes);
+ doWritePayloadLimitExceeded(String.format(Locale.ROOT,
+ "Estimated content size %d bytes exceeds IPC limit %d
bytes", estimated, maxPayloadBytes));
+ return;
+ }
+ }
+
+ byte[] bytes;
+ try {
+ bytes = JsonPipesIpc.toBytes(pipesResult);
+ } catch (OutOfMemoryError oom) {
+ // The large byte-builder segments are now GC-eligible; the tiny
error result below
+ // should serialize cleanly even on a depleted heap.
+ LOG.error("OOM during result serialization; returning
PAYLOAD_LIMIT_EXCEEDED", oom);
+ doWritePayloadLimitExceeded("OOM during result serialization: " +
oom.getMessage());
+ return;
+ }
+
+ // Post-check: serialized size may exceed the limit when content is
Unicode-heavy
+ // (the pre-check uses a 1 byte/char estimate; CJK chars use 3 bytes
in UTF-8 Smile).
+ if (bytes.length > maxPayloadBytes) {
+ LOG.warn("Serialized payload {} bytes exceeds maxIpcPayloadBytes
{}; returning PAYLOAD_LIMIT_EXCEEDED",
+ bytes.length, maxPayloadBytes);
+ doWritePayloadLimitExceeded(String.format(Locale.ROOT,
+ "Serialized payload %d bytes exceeds IPC limit %d bytes",
bytes.length, maxPayloadBytes));
+ return;
+ }
+
+ PipesMessage.finished(bytes).write(output);
+ awaitAck();
+ }
+
+ private void doWritePayloadLimitExceeded(String message) throws
IOException {
+ byte[] bytes = JsonPipesIpc.toBytes(
+ new
PipesResult(PipesResult.RESULT_STATUS.PAYLOAD_LIMIT_EXCEEDED, message));
PipesMessage.finished(bytes).write(output);
Review Comment:
The fallback frame also needs to fit `maxPayloadBytes` (or config validation
needs to enforce a minimum that guarantees it will). `setMaxIpcPayloadBytes()`
currently accepts any positive value, so with a small configured limit this
serialized `PAYLOAD_LIMIT_EXCEEDED` result is itself larger than the limit. The
production client then rejects it in `PipesMessage.read(...,
maxIpcPayloadBytes)`, closes the connection, and never receives the clean
status this path promises. `ServerProtocolIOTest.exchange()` masks this by
reading with `Integer.MAX_VALUE`. Please add a test where the client reads the
fallback using the same configured limit and either use a bounded/minimal error
payload or reject limits too small to carry it.
--
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]
