rpc: fix correlation_id overflow #16156
Merged
Conversation
This file contains 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
If we happen to end up in this exception handler, chances are that we never inserted an entry into _requests_queue. This means that there is a hole in the sequence numbers sequence and the connection is not usable anymore. Better to reset it.
To check that netbuf header is initialized, we checked correlation_id, treating 0 as special invalid value. But it turns out, we can get 0 if correlation_id overflows uint32_t (this can happen on a reasonably busy connection). The overflow itself is benign as correlation_id just wraps around to 0 (and it is unlikely that there still remain entries with these values in the _correlations map) but the check throws. Since it is just a precautionary check that doesn't add much value, remove it.
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/43922#018d1fa9-d4e6-4018-ac77-41ef6505cc54 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/43922#018d1fa9-d4df-4426-a257-a1ef5d476a7a |
mmaslankaprv
approved these changes
Jan 19, 2024
|
/backport v23.3.x |
|
/backport v23.2.x |
|
/backport v23.1.x |
|
Failed to create a backport PR to v23.1.x branch. I tried: |
This was referenced Jan 19, 2024
dotnwat
reviewed
Jan 19, 2024
Comment on lines
-35
to
-39
| if (hdr.correlation_id == 0 || hdr.meta == 0) { | ||
| throw std::runtime_error( | ||
| "cannot compose scattered view with incomplete header. missing " | ||
| "correlation_id or remote method id"); | ||
| } |
There was a problem hiding this comment.
in the commit message:
(and it is unlikely that there still remain entries with
these values in the _correlations map)
in the unlikely event this occurred, wouldn't this pair a response with the incorrect request?
There was a problem hiding this comment.
We have a check for this here
There was a problem hiding this comment.
ah yes of course i forgot. thanks!
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.
It is possible that in an active RPC client connection,
correlation_id(which is uint32_t) will overflow - at, say, 5k req/s (high, but not unreasonable value) overflow happens after 10 days. This isn't particularly bad, because it will just wrap around to 0, but we treatcorrelation_id=0as an invalid value and throw an exception. Because this exception gets thrown from an unexpected place, the connection send loop stalls and the connection becomes unusable afterwards.Remove this check as it doesn't add much value, and also ensure that connections get shut down after these unexpected exceptions.
Backports Required
Release Notes
Bug Fixes