erts: don't crash getprotocol/1 when the port has closed - #11438
Open
lukebakken wants to merge 1 commit into
Open
erts: don't crash getprotocol/1 when the port has closed#11438lukebakken wants to merge 1 commit into
lukebakken wants to merge 1 commit into
Conversation
prim_inet:sendfile/4 guards on erlang:port_info(S, connected), and on
success calls sendfile_maybe_cork/1 -> getprotocol/1, which does a
second, unguarded `{name,Drv} = erlang:port_info(S, name)`. If the peer
closes the socket between those two port_info/2 calls, the second
returns the atom `undefined` and the match raises {badmatch,undefined},
crashing the caller. A fully-closed socket instead fails the connected
guard and returns {error, einval}; only this race raises.
Match on the port_info/2 result and return `undefined` for a closed
port instead of badmatching. `undefined` is already in getprotocol/1's
return contract (drv2protocol/1 has a catch-all `undefined` clause), and
both call sites already tolerate it: sendfile_maybe_cork/1 falls through
to `_ -> false` (skip corking, so sendfile_1 returns {error, einval}
cleanly), and bindx/3 falls through to `_ -> {error, einval}`.
Reported downstream at rabbitmq/osiris#230, where the race surfaced as a
noisy osiris_replica_reader crash during high-throughput stream
replication over TCP.
Contributor
CT Test Results 3 files 136 suites 52m 28s ⏱️ Results for commit 95835d0. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
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.
Note
This PR was prepared by Claude (Anthropic's Claude Code) under the direction of @lukebakken, who reviewed the change before opening it. The underlying crash was surfaced by a 24-hour long-running high-throughput test. The code and analysis are AI-drafted and human-reviewed.
Problem
prim_inet:sendfile/4guards onerlang:port_info(S, connected), and on success callssendfile_maybe_cork/1 -> getprotocol/1, which does a second, unguarded{name,Drv} = erlang:port_info(S, name). If the peer closes the socket between those twoport_info/2calls, the second returns the atomundefinedand the match raises{badmatch,undefined}, crashing the caller.A fully-closed socket does not hit this: it fails the earlier
connectedguard andsendfile/4returns{error, einval}. Only a close that races the narrow window between the twoport_info/2calls reaches the bad match.Fix
Match on the
port_info/2result and returnundefinedfor a closed port instead of badmatching:undefinedis already part ofgetprotocol/1's return contract (drv2protocol/1has a catch-allundefinedclause), and both call sites already tolerate it:sendfile_maybe_cork/1falls through to_ -> false(skip corking, sosendfile_1proceeds and returns{error, einval}cleanly);bindx/3falls through to_ -> {error, einval}.On testing
I have not added a test case, and I want to be up front about why. To reach the fixed line a port must simultaneously (a) pass
sendfile/4'sport_info(S, connected)guard and (b) returnundefinedfromport_info(S, name). Those two states only coexist in the transient close window, so the condition is not reproducible on demand through any public API. A fully-closed socket, which is deterministic, returns{error, einval}before reachinggetprotocol/1, so a test built on that would pass with or without this fix.The only deterministic test I can see is a unit test calling
getprotocol/1directly on a closed port (returnsundefinedwith this fix, raises{badmatch,undefined}without it). That would require exportinggetprotocol/1fromprim_inet, and I did not want to widen a preloaded module's public surface without maintainer input. Happy to add that, or a test in whatever form you prefer, if you would like one.I did confirm the closed-port behavior empirically:
erlang:port_info(P, name)returns the bare atomundefinedfor a closed port, the pre-fix code raises{badmatch,undefined}on it, and the post-fix code returnsundefined. The fullsendfile_SUITEpasses (14/14) on a runtime built with this change.Origin
Reported downstream at rabbitmq/osiris#230, where the race surfaced as a noisy
osiris_replica_readercrash during high-throughput stream replication over TCP. The osiris side adds a defence-in-depth guard for current OTP releases in rabbitmq/osiris#231; this PR fixes the root cause in OTP.