Repository navigation
[#1355]Fix: reject non-Metric types when deserializing algorithm metrics - #1369
Open
NopeCharon wants to merge 1 commit into
Open
NopeCharon wants to merge 1 commit into
NopeCharon wants to merge 1 commit into
Conversation
…thm metrics MasterService.setAlgoMetrics passes the bytes received over RPC to KryoUtils.deserializeAlgoMetric, which called readClassAndObject and only cast the result to Metric afterwards. Any class named in the payload was therefore instantiated and populated first, and a forged primitive array header with a huge length made the Master allocate the whole array before reading a single element, crashing it with an OutOfMemoryError. Read the class header on its own and check that it is a Metric subclass before reading the object, throwing a KryoException otherwise. Custom Metric subclasses keep working because the check is by type rather than a fixed list of the bundled metrics. Since the class and the object are now read in two calls, auto reset is disabled on the thread-local Kryo used for deserialization and the graph state is reset once the whole metric has been read. Otherwise class name references inside the metric (for example a field of the metric's own class) could not be resolved. Add KryoUtilsTest covering round trips of the bundled metrics, a null metric, a metric that references its own class, and rejection of a non-metric object and of a forged long[] header. Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
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
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.
Pre-submission checklist:
Please check each of these after submitting your pull request:
[Issue ID]What this PR does
Fixes the deserialization part of #1355.
MasterService.setAlgoMetricshands the bytes from the RPC request toKryoUtils.deserializeAlgoMetric, which calledreadClassAndObjectand cast the result toMetriconly afterwards. Two things followed from that:long[]with a length nearInteger.MAX_VALUE) made the Master allocate the whole array before reading any element, and theOutOfMemoryErrortook down the Master.deserializeAlgoMetricnow reads the class header on its own, checks that it is aMetricsubclass, and only then reads the object withreadObject. Anything else is rejected with aKryoExceptionbefore an object is created. The check is by type, not a fixed list ofLossMetric/ErrorMetric/ObjMetric, so user-definedMetricsubclasses keep working.Because the class and the object are now read in two calls, auto reset is turned off on the thread-local
KryoReflectionFactorySupportused here and the graph state is reset in afinallyblock once the metric has been read. Without this, Kryo would clear its class name table right afterreadClass, and a metric whose fields refer back to its own class would fail withUnable to find class.The wire format is unchanged, so the serializing side (
MasterClient) is untouched.Not covered here: the RPC endpoint itself is still unauthenticated. The issue also suggests adding authentication, which is a much larger design change and is better handled separately.
Tests
New
KryoUtilsTest(JUnit 4, same as the other core tests):LossMetric,ErrorMetricandObjMetricnullmetricMetricobject is rejected withKryoExceptionlong[]header with a huge length is rejected withKryoExceptionHow I ran them: I compiled
KryoUtilsand the test with JDK 8 against the pinnedkryo-shaded4.0.0 andkryo-serializers0.42 jars, using the releasedangel-ps-core3.2.0 jar for theMetricclasses. I couldn't run the full Maven build becauseprotoc2.5.0 wasn't available on my build machine.deserializeAlgoMetric: the two rejection tests fail with the behaviour described in the issue (ClassCastExceptionfor the non-metric object,OutOfMemoryError: Requested array size exceeds VM limitfor the forged array).