pjfanning opened a new pull request, #3507: URL: https://github.com/apache/pekko/pull/3507
### Motivation Three places read repeated protobuf fields that `toBinary` writes in lockstep, but read them as if the lengths were guaranteed to agree. **`DaemonMsgCreateSerializer:191-201`** drives its loop over the constructor arguments by `getSerializerIdsCount` and then indexes `getArgs(idx)`, `getManifests(idx)` and `getHasManifest(idx)` with it. A message where those disagree raises `IndexOutOfBoundsException` out of the generated accessor. The pre-2.4 branch below it zips args with manifests, which silently drops the tail of the longer list instead. **`ArteryMessageSerializer:185-190`** zips the keys and values of a compression table advertisement. A mismatch silently builds a table the sender did not advertise — and this table becomes the node's outbound compression table (`ArteryTransport.scala:523`), which is then acknowledged back to the sender as accepted at the advertised version. **`ArteryMessageSerializer:190` and `:205`** narrow the advertised table version, and the ack's version, from `int` to `byte` with `byteValue` / `toByte`. `CompressionTable.version` is a `Byte` on both sides, so anything outside that range is not a version a peer advertised, and narrowing makes values 256 apart indistinguishable. I only spotted the ack site while writing the fix; it has the same problem. ### Modification Check the lengths agree before indexing, and check the version fits in a byte before narrowing, reporting either as `NotSerializableException`. Both are conditions no `toBinary` produces: `DaemonMsgCreateSerializer.toBinary` appends to all four lists per argument, and `serializeCompressionAdvertisement` adds a key and a value per entry from a `Byte` version. ### Result A malformed message is reported as a serialization failure rather than raising `IndexOutOfBoundsException` or being silently accepted as something other than what it said. ### Tests - `sbt "remote/testOnly org.apache.pekko.remote.serialization.*"` — 199 passed, 1 pending Five new tests, four of which were checked to discriminate by reverting the two production files and re-running. The pre-fix failures are worth recording because they confirm each diagnosis: | Test | Without the fix | | --- | --- | | reject a compression table advertisement whose keys and values disagree in length | **no exception** — the truncated table was accepted | | reject a compression table version that does not fit in a byte (advertisement and ack) | **no exception** — silently narrowed | | reject props whose repeated fields disagree in length | `IndexOutOfBoundsException` | | reject old format props whose args and manifests disagree in length | `ClassNotFoundException` — it zipped down to one pair, then tried to load the string manifest as a class name | The fifth, `accept the whole byte range of compression table versions`, passes either way by design: it is the no-regression guard for `Byte.MinValue`, `-1`, `0`, `1` and `Byte.MaxValue`, since `-1` means "disabled" and the range is documented on `CompressionTable`. - `sbt "remote/mimaReportBinaryIssues"` — no issues - `sbt "remote/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]
