Skip to content

fix: don't dereference a null superclass in the protobuf allow list check - #3509

Open
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:protobuf-allowlist-order
Open

fix: don't dereference a null superclass in the protobuf allow list check#3509
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:protobuf-allowlist-order

Conversation

@pjfanning

Copy link
Copy Markdown
Member

Motivation

This is the pekko-remote half of the allow list ordering noted in #3503, and looking at it
turned up something more than a reorder.

ProtobufSerializer.isInAllowListClassName (:196-200) reads clazz.getSuperclass.getName
unconditionally. getSuperclass is null for an interface, for java.lang.Object and for a
primitive, and the class being checked is the manifest that arrived on the wire — so a manifest
naming an interface raises NullPointerException out of the allow list check instead of the
IllegalArgumentException that refusing a class is supposed to produce. It is reachable
whenever the named class is not bound to a protobuf serializer, which is exactly the case the
allow list exists to refuse.

Separately, isInAllowList (:183-185) evaluated isBoundToProtobufSerializer first. That
calls serializerFor, which raises NotSerializableException — stack trace and all — for a
class that is not bound, which is the common case for a class allowed only by
pekko.serialization.protobuf.allowed-classes.

Modification

Skip the superclass when there is none, and test the name list before the binding lookup, which
cannot throw. Both operands are pure predicates, so which one runs first does not change the
decision.

The ordering matters less here than in Jackson: ProtobufSerializer caches its parsing handle
after the check (:124-133), so it pays the cost once per class rather than once per message.
The null dereference is the part worth fixing.

Result

An interface or Object manifest is refused with the allow list error rather than a
NullPointerException.

Tests

  • sbt "remote/testOnly org.apache.pekko.remote.serialization.*" — 196 passed, 1 pending

Two new tests in ProtobufSerializerSpec, both checked to discriminate by reverting the
production file and re-running — each then fails with NullPointerException was thrown:

  • reject an interface manifest rather than failing on its missing superclass

  • reject java.lang.Object as a manifest

  • sbt "remote/mimaReportBinaryIssues" — no issues

  • sbt "remote/scalafmtCheckAll" headerCreateAll — clean

References

Completes the isInAllowList ordering from #3503, which changed the two Jackson serializers and
deliberately left this one out of a Jackson-scoped PR.

…heck

Motivation:
ProtobufSerializer.isInAllowListClassName reads clazz.getSuperclass.getName
unconditionally. getSuperclass is null for an interface, for Object and for
a primitive, and the class it checks is the manifest from the wire, so a
manifest naming an interface raised NullPointerException from the allow
list check rather than the IllegalArgumentException that refusing a class
is meant to produce.

isInAllowList also evaluated isBoundToProtobufSerializer first, which calls
serializerFor and raises, filling in a stack trace, for a class that is not
bound - the common case for a class allowed only by name.

Modification:
Skip the superclass when there is none, and test the name list before the
binding, which cannot throw. Both operands are pure predicates so the
decision is unchanged.

Result:
An interface or Object manifest is refused with the allow list error
instead of a NullPointerException.
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