pjfanning opened a new pull request, #3508:
URL: https://github.com/apache/pekko/pull/3508
### Motivation
Gossip interns addresses, roles, hashes and app versions into tables and
refers to them by
index. Every one of those indexes went straight from the wire into
`Vector.apply` with no range
check:
| Site | Table |
| --- | --- |
| `ClusterMessageSerializer:551,554` | observer and subject addresses in
reachability |
| `:570` | member address |
| `:578` | member role |
| `:572` | member app version |
| `:588` | tombstone address |
| `:596` | seen addresses |
| `:605` | vector clock hash, reached from both `Gossip` and `GossipStatus` |
A negative or out of range index raises `IndexOutOfBoundsException` rather
than a serialization
failure. For a `GossipEnvelope` this lands somewhere unexpected:
`gossipEnvelopeFromProto`
(`:608-615`) defers the whole parse into a `() =>` thunk, so the throw
happens inside
`ClusterCoreDaemon` when the gossip is read, not on a deserialization thread
where a failure
would be contained.
### 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 by construction, so nothing a peer legitimately sends is
affected.
**Correction to what I reported earlier.** I had also flagged
`memberStatusFromInt` and
`reachabilityStatusFromInt` as reachable `NoSuchElementException`s on an
unknown enum number.
That is wrong: `ClusterMessages.proto` is `proto2`, where an unrecognised
enum value is moved to
the unknown fields and the accessor returns the default, so
`getStatus.getNumber` is always one
of the declared values and both maps are total over it. I had a check
written for those two and
removed it rather than ship defensive code that cannot fire and cannot be
tested.
### Result
Gossip that refers to a table entry the sender did not include is reported
as a serialization
failure naming the index and the size.
### Tests
- `sbt "cluster/testOnly org.apache.pekko.cluster.protobuf.*"` — 12 passed
Three new tests, each checked to discriminate by reverting the production
file and re-running;
all three then fail with `IndexOutOfBoundsException was thrown`:
- `reject gossip that refers to a lookup table entry it did not send` —
takes a real `Welcome`,
decompresses it, and tampers one index at a time: an address index past
the end, a negative
address index, a role index, a `seen` entry, and a vector clock hash index
- `reject a gossip status that refers to a hash it did not send` —
`GossipStatus` reaches
`vectorClockFromProto` without going through `Gossip`, and is not
compressed
- `reject a gossip envelope with a bad index when the gossip is read` —
asserts the failure
surfaces from `envelope.gossip`, not from `fromBinary`, which is where the
deferred parse puts it
- `sbt "cluster/mimaReportBinaryIssues"` — no issues
- `sbt "cluster/scalafmtCheckAll" headerCreateAll` — clean
### References
None.
--
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]
