fix: bounds check the lookup table indexes in gossip (#3508) - #3520
Open
pjfanning wants to merge 1 commit into
Open
fix: bounds check the lookup table indexes in gossip (#3508)#3520pjfanning wants to merge 1 commit into
pjfanning wants to merge 1 commit into
Conversation
Motivation: Gossip interns addresses, roles, hashes and app versions into tables and refers to them by index. Every index in gossipFromProto came straight off the wire into Vector.apply with no range check, so a negative or out of range one raised IndexOutOfBoundsException instead of a serialization failure. For a GossipEnvelope this matters more than usual: gossipEnvelopeFromProto defers the parse into a thunk, so the throw happens inside ClusterCoreDaemon when the gossip is read rather than on a deserialization thread. Modification: Look the indexes up through a helper that range checks first and reports a NotSerializableException naming the index and the table size. Every index gossipToProto writes is in range, so nothing a peer legitimately sends is affected. Result: Gossip that refers to a table entry the sender did not include is reported as a serialization failure.
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
1 participant
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.
Motivation
Backport of #3508 to 1.7.x: gossip lookup-table indexes went from the wire straight into
Vector.apply, so an out-of-range index raisedIndexOutOfBoundsException— and for aGossipEnvelopethe deferred parse puts that throw inside the cluster daemon rather thanon a deserialization thread.
Modification
Cherry-pick of the #3508 change commit. The only conflict was the import block of
ClusterMessageSerializerSpec(theNotSerializableExceptionimport); everything else,including the new
lookuphelper, applied clean — the touched.asScala...to(...)linesare identical on 1.7.x, where the file already imports
pekko.util.ccompat.JavaConverters.Note #3508 is still open against
main; if review there changes the fix, this backportshould be updated to match before merging.
Result
Same as #3508: gossip referring to a table entry the sender did not include is reported
as a serialization failure naming the index and the table size.
Tests
sbt "++ 2.12.21 cluster/Test/compile"— clean, validating Scala 2.12sbt "cluster/scalafmtCheckAll"— cleanfrom fix: bounds check the lookup table indexes in gossip #3508 are included, covering
Gossip,GossipStatus, and the deferredGossipEnvelopepathReferences
Backport of #3508.