Skip to content

fix: reject messages whose parallel repeated fields disagree in length - #3507

Open
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:artery-compression-advert
Open

fix: reject messages whose parallel repeated fields disagree in length#3507
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:artery-compression-advert

Conversation

@pjfanning

Copy link
Copy Markdown
Member

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.

Motivation:
Three places read repeated protobuf fields that are written in lockstep but
read as if their lengths were guaranteed to agree.

DaemonMsgCreateSerializer drives its loop over the constructor arguments by
getSerializerIdsCount and indexes args, manifests and hasManifest with it,
so a message where those disagree raises IndexOutOfBoundsException. The
pre-2.4 branch zips args with manifests, which silently drops the tail of
the longer one.

ArteryMessageSerializer zips the keys and values of a compression table
advertisement, so a mismatch silently builds a table the sender did not
advertise, which is then acknowledged back to the sender as accepted. It
also narrows the advertised table version, and the ack's version, from int
to byte with byteValue, so versions 256 apart are indistinguishable.

Modification:
Check the lengths agree before indexing, and check the table version fits
in a byte before narrowing it. Report either as NotSerializableException.
Both are conditions no toBinary produces.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant