HADOOP-19864 Followup: rejection of unregistered protocols.#8470
Open
steveloughran wants to merge 1 commit intoapache:trunkfrom
Open
HADOOP-19864 Followup: rejection of unregistered protocols.#8470steveloughran wants to merge 1 commit intoapache:trunkfrom
steveloughran wants to merge 1 commit intoapache:trunkfrom
Conversation
Server will immediately reject any protocols for which no handler is registered. Contains content written by github copilot
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens the IPC/RPC server by rejecting RPC kinds that do not have any protocols registered on the server, aiming to avoid deserializing untrusted request payloads for unsupported kinds.
Changes:
- Add an early rejection path in
Server.processRpcRequest()when aRPC.Serverhas no registered protocols for the requestRpcKind. - Introduce
RPC.Server#hasRegisteredProtocols(RpcKind)to detect whether a kind is supported on a given server instance. - Add a new unit test intended to validate the “unregistered kind” behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java |
Adds pre-deserialization rejection for RPC kinds without registered protocols; adjusts logging/error strings. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java |
Adds hasRegisteredProtocols helper to support the early-rejection check. |
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestRPC.java |
Adds a test case intended to cover rejection of unregistered RPC kinds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2119
to
+2142
| /** | ||
| * 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(); | ||
| } | ||
| } |
| final String err = "No protocols registered on this server for RpcKind " | ||
| + header.getRpcKind() | ||
| + ". Rejecting request without deserialization."; | ||
| LOG.info("{} Client: {}", err, getHostAddress()); |
Contributor
Author
There was a problem hiding this comment.
rejected as upgrading to warn is even noisier. We could go to LogExactlyOnce
| 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 {}", |
| throw new FatalRpcServerException( | ||
| RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, err); | ||
| RpcErrorCodeProto.FATAL_INVALID_RPC_HEADER, | ||
| "Unknown rpc kind in rpc header" + header.getRpcKind()); |
cnauroth
approved these changes
May 5, 2026
Contributor
cnauroth
left a comment
cnauroth
left a comment
There was a problem hiding this comment.
+1, pending pre-commits
|
🎊 +1 overall
This message was automatically generated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Followup to #8433.
Server will immediately reject any protocols for which no handler is registered.
How was this patch tested?
new test case
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html