Copilot commented on code in PR #8470:
URL: https://github.com/apache/hadoop/pull/8470#discussion_r3191079115
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java:
##########
@@ -2946,28 +2946,40 @@ private void processRpcRequest(RpcRequestHeaderProto
header,
throw new FatalRpcServerException(
RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, err);
}
- Class<? extends Writable> rpcRequestClass =
+ // Reject requests for RPC kinds with no registered protocols on this
+ // server instance. This prevents deserialization of untrusted payloads
+ // for unsupported kinds. See HADOOP-19864.
+ if (Server.this instanceof RPC.Server server) {
+ final RPC.RpcKind kind = ProtoUtil.convert(header.getRpcKind());
+ if (!server.hasRegisteredProtocols(kind)) {
+ final String err = "No protocols registered on this server for
RpcKind "
+ + header.getRpcKind()
+ + ". Rejecting request without deserialization.";
+ LOG.info("{} Client: {}", err, getHostAddress());
Review Comment:
Logging this rejection at INFO may be too noisy in production if clients
probe/scan with unsupported kinds (it can become log spam). Consider switching
to WARN (to align with the existing \"Unknown rpc kind\" handling) and/or using
a rate-limited logger for this specific message.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java:
##########
@@ -2946,28 +2946,40 @@ private void processRpcRequest(RpcRequestHeaderProto
header,
throw new FatalRpcServerException(
RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, err);
}
- Class<? extends Writable> rpcRequestClass =
+ // Reject requests for RPC kinds with no registered protocols on this
+ // server instance. This prevents deserialization of untrusted payloads
+ // for unsupported kinds. See HADOOP-19864.
+ if (Server.this instanceof RPC.Server server) {
+ final RPC.RpcKind kind = ProtoUtil.convert(header.getRpcKind());
+ if (!server.hasRegisteredProtocols(kind)) {
+ final String err = "No protocols registered on this server for
RpcKind "
+ + header.getRpcKind()
+ + ". Rejecting request without deserialization.";
+ LOG.info("{} Client: {}", err, getHostAddress());
+ throw new FatalRpcServerException(
+ RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, err);
+ }
+ }
+ Class<? extends Writable> rpcRequestClass =
getRpcRequestWrapper(header.getRpcKind());
if (rpcRequestClass == null) {
- LOG.warn("Unknown rpc kind " + header.getRpcKind() +
- " from client " + getHostAddress());
- final String err = "Unknown rpc kind in rpc header" +
- header.getRpcKind();
+ LOG.warn("Unknown rpc kind {} from client {}", header.getRpcKind(),
getHostAddress());
throw new FatalRpcServerException(
- RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, err);
+ RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER,
+ "Unknown rpc kind in rpc header" + header.getRpcKind());
}
Writable rpcRequest;
try { //Read the rpc request
rpcRequest = buffer.newInstance(rpcRequestClass, conf);
} catch (RpcServerException rse) { // lets tests inject failures.
throw rse;
} catch (Throwable t) { // includes runtime exception from newInstance
- LOG.warn("Unable to read call parameters for client " +
- getHostAddress() + "on connection protocol " +
- this.protocolName + " for rpcKind " + header.getRpcKind(), t);
- String err = "IPC server unable to read call parameters: "+
t.getMessage();
+ LOG.warn(
+ "Unable to read call parameters for client {}on connection
protocol {} for rpcKind {}",
Review Comment:
The log message is missing a space after the first placeholder (`{}`),
producing `client <addr>on connection...`. Add a space so the message reads
`client {} on connection protocol ...`.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java:
##########
@@ -2946,28 +2946,40 @@ private void processRpcRequest(RpcRequestHeaderProto
header,
throw new FatalRpcServerException(
RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, err);
}
- Class<? extends Writable> rpcRequestClass =
+ // Reject requests for RPC kinds with no registered protocols on this
+ // server instance. This prevents deserialization of untrusted payloads
+ // for unsupported kinds. See HADOOP-19864.
+ if (Server.this instanceof RPC.Server server) {
+ final RPC.RpcKind kind = ProtoUtil.convert(header.getRpcKind());
+ if (!server.hasRegisteredProtocols(kind)) {
+ final String err = "No protocols registered on this server for
RpcKind "
+ + header.getRpcKind()
+ + ". Rejecting request without deserialization.";
+ LOG.info("{} Client: {}", err, getHostAddress());
+ throw new FatalRpcServerException(
+ RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, err);
+ }
+ }
+ Class<? extends Writable> rpcRequestClass =
getRpcRequestWrapper(header.getRpcKind());
if (rpcRequestClass == null) {
- LOG.warn("Unknown rpc kind " + header.getRpcKind() +
- " from client " + getHostAddress());
- final String err = "Unknown rpc kind in rpc header" +
- header.getRpcKind();
+ LOG.warn("Unknown rpc kind {} from client {}", header.getRpcKind(),
getHostAddress());
throw new FatalRpcServerException(
- RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, err);
+ RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER,
+ "Unknown rpc kind in rpc header" + header.getRpcKind());
Review Comment:
The thrown error string concatenates without spacing (`... rpc
header<kind>`). Add a separating space (or use structured formatting) so the
message is clearer, e.g., `\"Unknown rpc kind in rpc header \" +
header.getRpcKind()`.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestRPC.java:
##########
@@ -2116,6 +2116,31 @@ public void testNumTotalRequestsMetrics() throws
Exception {
}
+ /**
+ * Test that a Protobuf-only RPC server rejects requests for RpcKinds
+ * that have no registered protocols, without deserializing the payload.
+ */
+ @Test
+ @Timeout(value = 30)
+ public void testUnregisteredRpcKindRejectedWithoutDeserialization()
+ throws Exception {
+ // Standard test server: only RPC_PROTOCOL_BUFFER protocols are registered.
+ RPC.Server server = setupTestServer(conf, 1);
+ try {
+ // RPC_PROTOCOL_BUFFER has registered protocols — must be accepted.
+
assertThat(server.hasRegisteredProtocols(RPC.RpcKind.RPC_PROTOCOL_BUFFER))
+ .as("RPC_PROTOCOL_BUFFER should have registered protocols")
+ .isTrue();
+
+ // RPC_BUILTIN has no protocols registered on this server — must be
rejected.
+ assertThat(server.hasRegisteredProtocols(RPC.RpcKind.RPC_BUILTIN))
+ .as("RPC_BUILTIN should have no registered protocols on a
Protobuf-only server")
+ .isFalse();
+ } finally {
+ server.stop();
+ }
+ }
Review Comment:
This test only asserts server-side state (`hasRegisteredProtocols`) and does
not exercise the new behavior in `Server.processRpcRequest()` (i.e., rejecting
an incoming request before deserialization). To validate the regression fix,
update the test to send an actual RPC request with an unsupported `RpcKind` and
assert the server rejects it (and, ideally, that deserialization is not
invoked—e.g., by using a payload that would fail/trigger if deserialized or by
instrumenting/spy points where request deserialization would occur).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
