Skip to content

fix: harden complex parameter model loading - #2678

Open
Rana Singh (ranadeepsingh) wants to merge 10 commits into
microsoft:masterfrom
ranadeepsingh:fix/serializer-deserialization-policy
Open

fix: harden complex parameter model loading#2678
Rana Singh (ranadeepsingh) wants to merge 10 commits into
microsoft:masterfrom
ranadeepsingh:fix/serializer-deserialization-policy

Conversation

@ranadeepsingh

@ranadeepsingh Rana Singh (ranadeepsingh) commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Related Issues/PRs

Follow-up to #2513 and the remaining MSRC recommendation in IcM 31000000568481.

What changes are proposed in this pull request?

This PR closes the remaining unrestricted Java deserialization path in
ComplexParams model persistence. A crafted artifact could previously execute
deserialization callbacks before the eventual type cast.

The patch:

  • fails closed for unconstrained Java object graphs and permits only audited,
    type-specific class policies;
  • rejects serialized lambdas and unsafe proxies and enforces graph, object,
    array, string, stream, metadata, path, and heap-aware limits;
  • migrates data types and nested stages to bounded formats/readers where
    possible;
  • validates model metadata, canonical artifact containment, aliases, links,
    and bounded remote enumeration before reader construction;
  • scopes trusted legacy compatibility to the active Spark session and explicit
    artifact load;
  • binds SynapseML Python readers to the active session and uses the bounded
    PipelineSerializer for generated nested-stage tests; and
  • centralizes trust for harness-created fixtures instead of adding
    suite-specific opt-outs.

Compatibility contract

New data-only parameter artifacts load without unsafe Java deserialization.
Legacy artifacts containing arbitrary closures, BallTrees, DataFrames, native
readers, or native Spark Pipeline nesting must be explicitly treated as
trusted by setting
spark.synapseml.legacy.allowUnsafeJavaDeserialization=true on the reader's
Spark session. Native Pipeline compatibility must additionally run inside
Serializer.withTrustedArtifactLoad; untrusted nested models use SynapseML's
bounded PipelineSerializer.

Direct ConditionalBallTree.load fails closed. Trusted legacy callers can use
the explicit loadUnsafe API.

Scope

The effective diff contains 20 connected production security-boundary files
and 25 focused test/harness files. It contains no review artifacts, dependency
changes, workflow changes, or generated target/ files.

How is this patch tested?

  • Master baseline (JDK 11 / Scala 2.12): affected module main/test
    compilation and Scalastyle.
  • Core exploit, resource-budget, metadata/path, session-isolation,
    Pipeline/ComplexParams, generated-loader, UDF, recommendation, and
    persistence suites.
  • LightGBM constrained-deserialization test on Scala 2.12.
  • Generated persistence runtime tests: core (2), cognitive (1), and deep
    learning (3), including trust cleanup assertions.
  • Core, cognitive, deep-learning, and OpenCV code/test generation plus
    generated Python syntax compilation.
  • Black 22.3.0 and git diff --check.
  • Exact Azure failure reproductions: 59 focused tests on both the master and
    Spark 4.1 baselines, Spark ML-compatible generated R metadata paths, and the
    session-scoped SAR trusted round trip with trust restoration.
  • Spark 4.1 replay (JDK 17 / Scala 2.13): all affected module test
    compilation, 86 focused core tests, and the LightGBM constrained
    deserialization test.

Does this PR change any dependencies?

  • No. You can skip this section.
  • Yes. Make sure the dependencies are resolved correctly, and list changes here.

Does this PR add a new feature? If so, have you added samples on website?

  • No. You can skip this section.
  • Yes. Make sure you have added samples following below steps.

SynapseML CI and others added 2 commits August 26, 2026 02:32
## Summary
Fail closed when a ComplexParam object graph has no constrained deserialization policy, add per-type class filters for data-only parameters, and validate model metadata before loading payloads. Replace Java object streams with Spark Pipeline persistence for estimator and transformer arrays while retaining an explicit trusted-legacy compatibility path.

## Prompting Intent
Investigate whether unrestricted Serializer.read behavior remained security-relevant after the earlier BallTree mitigation, close any reachable storage-to-compute code-execution paths, and preserve compatibility for trusted legacy artifacts without weakening secure defaults.

## Linked Sources
- Prior partial mitigation: microsoft#2513

## Rationale
Per-type filters keep legitimate data-only object graphs loadable while preventing arbitrary classes and SerializedLambda callbacks from reaching readObject. Stage arrays use Spark's native persistence rather than another Java-serialization allowlist. Types that inherently capture executable closures fail closed and require a strongly named SparkSession opt-in so legacy compatibility is deliberate and auditable. Exact model-class and parameter-path checks prevent metadata pivots and payload redirection before deserialization begins.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary
Complete the remaining model-persistence hardening with fail-closed Java
deserialization, type-specific policies, bounded metadata and path handling,
session-scoped trust, safe DataType JSON persistence, and explicit trusted
compatibility for legacy native Spark Pipeline artifacts.

## Prompting Intent
Investigate the remaining MSRC recommendation after the earlier public fix,
determine whether unrestricted ComplexParam deserialization was still
reachable, remediate the complete persistence boundary, preserve an explicit
trusted-legacy path where safe migration is impractical, and provide extensive
local regression coverage without exposing private incident details.

## Linked Sources
- Prior public remediation: microsoft#2513
- Review evidence: reviews/serializer-deserialization/task-2513-attempt-1-review-1-gpt-5.6-sol.md

## Rationale
Java deserialization callbacks execute before casts or parameter validation,
and closure-bearing parameters cannot be secured with broad package
allowlists. The implementation therefore defaults to non-executable formats
or narrowly constrained object graphs, fails closed for arbitrary legacy
payloads, and requires an explicit trusted scope for compatibility. Shared
path, graph, stream, and metadata budgets address traversal, aliasing,
compression, and resource-exhaustion risks across nested model graphs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 26, 2026 10:04
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@github-actions

Copy link
Copy Markdown

Hey Rana Singh (@ranadeepsingh) 👋!
Thank you so much for contributing to our repository 🙌.
Someone from SynapseML Team will be reviewing this pull request soon.

We use semantic commit messages to streamline the release process.
Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix.
This helps us to create release messages and credit you for your hard work!

Examples of commit messages with semantic prefixes:

  • fix: Fix LightGBM crashes with empty partitions
  • feat: Make HTTP on Spark back-offs configurable
  • docs: Update Spark Serving usage
  • build: Add codecov support
  • perf: improve LightGBM memory usage
  • refactor: make python code generation rely on classes
  • style: Remove nulls from CNTKModel
  • test: Add test coverage for CNTKModel

To test your commit locally, please follow our guild on building from source.
Check out the developer guide for additional guidance on testing your change.

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens SynapseML/SparkML model persistence and loading paths to fail closed against unsafe Java deserialization, unbounded metadata/object graphs, and path-traversal/link/glob issues—while preserving an explicit trusted-legacy compatibility switch for artifacts that cannot be safely constrained.

Changes:

  • Introduces a session-scoped “trusted legacy artifact” gate (spark.synapseml.legacy.allowUnsafeJavaDeserialization) and routes unsafe persistence (UDF closures, custom readers, BallTrees, DataFrames, etc.) behind explicit opt-in.
  • Adds hardened, budgeted model metadata/path handling (canonical containment, link resolution, bounded metadata enumeration and decoding budgets, recursive model context) for Pipeline/ComplexParams loads.
  • Migrates DataTypeParam persistence toward bounded JSON handling and adds/updates extensive regression coverage for security and compatibility.
Show a summary per file
File Description
reviews/serializer-deserialization/task-2513-attempt-1-review-1-gpt-5.6-sol.md Adds a long-form review artifact documenting the hardening work and verification steps.
lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/params/VerifyLightGBMBoosterParam.scala Adds a regression ensuring LightGBM booster param loads under constrained legacy handling.
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMBoosterParam.scala Adds a narrow deserialization class policy for LightGBMBoosterParam.
core/src/test/scala/org/apache/spark/ml/VerifyArtifactPathResolver.scala Adds tests for metadata listing guarantees and Java 8 linkage constraints.
core/src/test/scala/com/microsoft/azure/synapse/ml/stages/UDFTransformerSuite.scala Adds trust-gating tests for persisted UDFs and session propagation through PipelineSerializer.
core/src/test/scala/com/microsoft/azure/synapse/ml/stages/LambdaSuite.scala Enables trusted legacy load path for serialization fuzzing where needed.
core/src/test/scala/com/microsoft/azure/synapse/ml/recommendation/RankingTrainValidationSpec.scala Enables trusted legacy load path for serialization fuzzing in ranking suites.
core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyEvaluatorParam.scala Adds explicit-trust tests for evaluator param persistence.
core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyEstimatorArrayParam.scala Adds coverage for stage-array persistence (safe writable vs trusted legacy cases).
core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyDataTypeParam.scala Adds coverage for JSON DataType persistence and legacy-stream gating/tripwires.
core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyDataFrameParam.scala Adds regression guarding against linked Parquet parts escaping artifact containment.
core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyArrayParamMapParam.scala Adds explicit-trust coverage for ArrayParamMapParam persistence.
core/src/test/scala/com/microsoft/azure/synapse/ml/nn/VerifySchemas.scala Extends SafeObjectInputStream/BallTree coverage and validates fail-closed NN loading defaults.
core/src/test/scala/com/microsoft/azure/synapse/ml/io/split1/ParserSuite.scala Enables trusted legacy load path for serialization fuzzing in parser suites where applicable.
core/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/VerifySafeObjectInputStream.scala Adds focused unit tests for SafeObjectInputStream resource and policy enforcement.
core/src/test/scala/com/microsoft/azure/synapse/ml/core/test/fuzzing/Fuzzing.scala Adds a controlled trusted-legacy mode for serialization fuzzing and Pipeline/PipelineModel round trips.
core/src/test/scala/com/microsoft/azure/synapse/ml/core/serialize/VerifyModelLoadEnvironment.scala Adds environment regressions around session scoping, trusted loading, and provider behaviors.
core/src/test/scala/com/microsoft/azure/synapse/ml/core/serialize/VerifyMetadataBudgets.scala Adds regression ensuring aggregate decoded-metadata budgets are enforced.
core/src/test/scala/com/microsoft/azure/synapse/ml/core/serialize/ValidateComplexParamSerializer.scala Significantly expands hardening regressions across metadata, paths, trust gates, and reader safety.
core/src/test/scala/com/microsoft/azure/synapse/ml/automl/VerifyFindBestModel.scala Enables trusted legacy load path for serialization fuzzing in AutoML persistence tests.
core/src/main/scala/org/apache/spark/ml/StageReaderInspector.scala Adds bytecode-based reader classification without class initialization.
core/src/main/scala/org/apache/spark/ml/Serializer.scala Centralizes hardened serializer routing, trust scoping, Pipeline serializers, and safe read/write behavior.
core/src/main/scala/org/apache/spark/ml/ModelLoadContext.scala Introduces shared load/write budgets (nodes, depth, metadata physical/decoded) across nested model loads.
core/src/main/scala/org/apache/spark/ml/DataTypeSerializer.scala Implements bounded DataType JSON persistence with legacy-stream detection and UDT gating.
core/src/main/scala/org/apache/spark/ml/ComplexParamsSerializer.scala Hardens ComplexParams read/write (session assignment, budgets, metadata accounting, and native pipeline boundaries).
core/src/main/scala/org/apache/spark/ml/ArtifactPathResolver.scala Adds canonical containment, link resolution, bounded metadata enumeration/decoding, and safe directory validation helpers.
core/src/main/scala/com/microsoft/azure/synapse/ml/param/TransformerArrayParam.scala Persists transformer arrays via hardened PipelineArraySerializer and validates load-time types.
core/src/main/scala/com/microsoft/azure/synapse/ml/param/EstimatorArrayParam.scala Persists estimator arrays via hardened PipelineArraySerializer and validates load-time types.
core/src/main/scala/com/microsoft/azure/synapse/ml/param/DataTypeParam.scala Adjusts DataTypeParam imports to align with new DataTypeSerializer path.
core/src/main/scala/com/microsoft/azure/synapse/ml/param/DataFrameParam.scala Marks DataFrameParam as not supporting untrusted deserialization by default.
core/src/main/scala/com/microsoft/azure/synapse/ml/param/ByteArrayParam.scala Adds an explicit deserialization class policy (primitive byte arrays).
core/src/main/scala/com/microsoft/azure/synapse/ml/param/BallTreeParam.scala Marks BallTree params as requiring trusted loading (fail closed by default).
core/src/main/scala/com/microsoft/azure/synapse/ml/nn/BallTree.scala Changes ConditionalBallTree default load to fail closed and adds explicit loadUnsafe.
core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/SafeObjectInputStream.scala Expands SafeObjectInputStream with resource limits, filter composition, and class allowlisting model.
core/src/main/scala/com/microsoft/azure/synapse/ml/core/serialize/ComplexParam.scala Adds per-param deserialization policy hooks and trust gating for unsafe complex params.

Review details

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 34/35 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread reviews/serializer-deserialization/task-2513-attempt-1-review-1-gpt-5.6-sol.md Outdated
## Summary
Clarify class-policy rejection messaging, remove machine-local paths from the
review evidence, and isolate new UDF test imports so the security patch replays
cleanly onto the spark4.1 compatibility branch.

## Prompting Intent
Resolve every current-head pull-request comment and the release-branch
compatibility failure without weakening the deserialization policy or changing
branch-specific Spark 4.1 UDF behavior.

## Linked Sources
- Pull request: microsoft#2678
- Class-policy feedback: microsoft#2678 (comment)
- Review-artifact feedback: microsoft#2678 (comment)
- Failed compatibility build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=232896880

## Rationale
The class policy supports both exact names and package prefixes, so the error
must describe the combined policy. Repo-relative evidence paths avoid leaking
workstation details. Moving imports into the added tests keeps the functional
master change intact while avoiding overlap with spark4.1's branch-only UDF
helper object; a local three-way replay confirmed the complete patch applies
cleanly to the current spark4.1 tip.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 10:23
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 35/35 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread core/src/main/scala/org/apache/spark/ml/ComplexParamsSerializer.scala Outdated
## Summary
Express Spark text framing from its explicit newline bytes and rename two
persistence tests so their names identify the parameter type under test.

## Prompting Intent
Resolve the current-head review comments precisely while preserving Spark
3.5's actual text-output framing semantics and keeping the security boundary
tests understandable across master and spark4.1.

## Linked Sources
- Pull request: microsoft#2678
- Framing review: microsoft#2678 (comment)
- Evaluator test review: microsoft#2678 (comment)
- ParamMap test review: microsoft#2678 (comment)
- Spark 3.5 TextOptions: https://github.com/apache/spark/blob/v3.5.0/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/text/TextOptions.scala

## Rationale
Spark's text writer does not use the platform line separator by default; its
TextOptions contract explicitly uses UTF-8 newline bytes. Computing the byte
length from that literal documents the dependency without introducing
incorrect Windows-specific accounting. Accurate test names improve failure
diagnostics without changing coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 10:40
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

core/src/main/scala/org/apache/spark/ml/Serializer.scala:88

  • typeToSerializer routes any Array[_] whose element type is a subtype of PipelineStage to PipelineArraySerializer and then casts it to Serializer[T]. Since arrays are reified on the JVM, PipelineArraySerializer will return an Array[PipelineStage], which cannot be safely treated as (for example) Array[Estimator[_]]/Array[Transformer] and can produce ClassCastException for any caller that goes through Serializer.typeToSerializer (outside the specialized Param overrides).

Consider returning a small adapter serializer for pipeline-stage arrays that converts to/from Array[PipelineStage] while preserving the requested runtime component type.

  def typeToSerializer[T](
      tpe: Type,
      sparkSession: SparkSession,
      classFilter: Option[DeserializationClassFilter]): Serializer[T] = {
    (if (tpe <:< typeOf[PipelineStage])              new PipelineSerializer(sparkSession)
     else if (isPipelineStageArray(tpe))             new PipelineArraySerializer(sparkSession)

core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/SafeObjectInputStream.scala:218

  • SafeObjectInputStream.ResourceFilter is a shared singleton instance of DeserializationResourceFilter, but DeserializationResourceFilter is stateful (declaredArrayBytes accumulates across checkInput calls). This makes the exposed filter instance easy to misuse and can make future tests order-dependent if they call ResourceFilter.checkInput on multiple “allowed” inputs.

Prefer exposing a factory (e.g., def newResourceFilter(...)) that returns a fresh per-stream filter, and update VerifySafeObjectInputStream to construct a new instance for assertions.

  private[utils] val ResourceFilter: ObjectInputFilter =
    new DeserializationResourceFilter(defaultResourceLimits)
  • Files reviewed: 35/35 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

## Summary
Preserve the requested JVM component type when generic serializers load
PipelineStage arrays, provide fresh stateful resource filters per stream, and
disambiguate Spark Transformer from Scala reflection for Spark 4.1 builds.

## Prompting Intent
Resolve all suppressed current-head review findings and the spark4.1
compatibility compile failure without weakening the secure Pipeline serializer
or its model-wide resource controls.

## Linked Sources
- Pull request: microsoft#2678
- Suppressed current-head review body: microsoft#2678
- Failed spark4.1 build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=232900732

## Rationale
JVM arrays are reified, so returning `Array[PipelineStage]` through a generic
`Serializer[Array[Estimator[_]]]` cast is not type safe. A small adapter now
validates elements and allocates the exact requested component array. Resource
filters track aggregate array bytes and therefore must never be shared across
streams. Fully qualifying Spark's Transformer avoids a Scala 2.13 reflection
name collision while remaining source-compatible with the master baseline.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 11:19
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

Resolved both suppressed findings from the 9aa866acbd automated review in 0e360a5827:

  • Generic PipelineStage-array serializers now allocate and validate the requested reified component type, with direct Array[Estimator[_]] and Array[Transformer] round-trip coverage.
  • DeserializationResourceFilter is now created fresh per stream/test instead of exposing a shared stateful singleton.
  • The same commit fully qualifies Spark Transformer, and the complete patch now passes spark4.1 core/Test/compile locally under Java 17 / Scala 2.13.

Master core compilation, main/test Scalastyle, VerifyEstimatorArrayParam, VerifySafeObjectInputStream, and ValidateComplexParamSerializer all pass.

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 36/36 changed files
  • Comments generated: 1
  • Review effort level: Lite

## Summary
Use overflow-safe multiplication and addition for declared array byte accounting, rejecting arithmetic overflow before it can weaken the deserialization resource budget. Add regressions for both per-array multiplication and aggregate addition overflow.

## Prompting Intent
Resolve the current-head automated review finding on the model deserialization hardening PR while preserving fail-closed JEP 290 resource enforcement and cross-version compatibility.

## Linked Sources
- Pull request: microsoft#2678
- Review comment: microsoft#2678 (comment)
- Prior remediation: microsoft#2513

## Rationale
Exact arithmetic makes overflow an explicit rejection instead of allowing signed Long wraparound. This keeps the stateful aggregate budget monotonic and avoids saturation logic that could obscure malformed stream accounting.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

## Summary
Add a fail-closed JEP 290 adapter that selects the Java 9+ java.io API or the Java 8 sun.misc backport at runtime without linking the published Scala sources to either interface. Keep per-stream class, graph, stream, and aggregate array limits active across supported Java versions.

## Prompting Intent
Resolve the exact-head Azure Publish compilation failure under Temurin 8 without weakening the model deserialization hardening or the overflow fix requested by automated review, while retaining Spark 3.5 and spark4.1 compatibility.

## Linked Sources
- Pull request: microsoft#2678
- Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=232908468&view=logs&jobId=0ccccc7a-9630-5914-467b-15a9c61f0287
- Overflow review comment: microsoft#2678 (comment)
- JEP 290: https://openjdk.org/jeps/290

## Rationale
Isolating runtime API differences behind a dynamic proxy preserves Java 8 source compatibility and per-stream JEP 290 enforcement on both API packages. This is safer than dropping resource filtering or changing the publication toolchain; runtimes without either supported filter API fail before reading an object graph.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 12:55
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

Resolved the exact-head Azure Publish failure from build 232908468 in 0036b77c5b.

The job used Temurin 8 and failed before publishing because java.io.ObjectInputFilter is a Java 9+ API. The fix isolates JEP 290 API differences behind a fail-closed runtime adapter: Java 9+ uses java.io.ObjectInputFilter, Java 8 uses the sun.misc.ObjectInputFilter backport, and runtimes without either abort before object-graph loading.

Current-head evidence:

  • Temurin 8u504: core/compile, core/Test/compile, and all 8 VerifySafeObjectInputStream tests pass, including a real JEP 290 rejection.
  • JDK 11 / Spark 3.5: core main/test compile, main/test Scalastyle, 37 end-to-end deserialization tests pass.
  • JDK 17 / Scala 2.13 / spark4.1 replay: core/Test/compile and all 8 filter tests pass.
  • No compile-time ObjectInputFilter reference remains; only runtime class-name strings are used.

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

core/src/test/scala/org/apache/spark/ml/VerifyArtifactPathResolver.scala:79

  • The InputStream is closed only on the happy path. If an assertion fails or an exception occurs during the read loop, the stream will leak and can hold open jar/file handles. Wrap the body in try/finally so input.close() always runs.
  • Files reviewed: 37/37 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

## Summary
Scope the ArtifactPathResolver class-resource stream with the repository using helper so assertion or read failures cannot leak file or jar handles.

## Prompting Intent
Resolve the current-head suppressed automated review finding without changing the Java 8 linkage assertion or production behavior, and preserve cross-version test compatibility.

## Linked Sources
- Pull request: microsoft#2678
- Automated review head: microsoft@0036b77

## Rationale
Reusing StreamUtilities.using follows the existing test convention and guarantees closure on both success and failure without duplicating manual try/finally cleanup.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 13:16
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

Resolved the suppressed 0036b77c5b review finding in f108242bd0: the Java 8 class-linkage test now scopes its resource stream with StreamUtilities.using, so read or assertion failures cannot leak file/jar handles.

VerifyArtifactPathResolver (6 tests), test compilation, and test Scalastyle pass on both Spark 3.5/JDK 11 and the spark4.1/Scala 2.13/JDK 17 replay.

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 37/37 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

## Summary
Centralize trusted handling for test-generated model fixtures, bind Python model readers to the active Spark session, correct mismatched test readers, and route security coverage through executed CI shards. Remove the obsolete review artifact and support Scala 2.13 singleton serialization within the constrained LightGBM policy.

## Prompting Intent
The engineer asked to finish the remaining MSRC deserialization remediation as a well-tested pull request, resolve review and CI failures, and reduce the number of changed files without weakening the security boundary.

## Linked Sources
- MSRC incident and remaining-fix assessment: https://portal.microsofticm.com/imp/v5/incidents/details/31000000568481/msrc
- Original partial remediation: microsoft#2513
- Follow-up pull request: microsoft#2678

## Rationale
Central fixture scoping replaces dozens of suite-specific trust flags while keeping production defaults fail-closed. Session binding is implemented once in JavaMMLReader rather than duplicated across generated wrappers. Native nested stages use the bounded PipelineSerializer, and Scala 2.13 compatibility permits only the exact serialization proxy while the referenced singleton class remains subject to the per-type allowlist.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 43/43 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

## Summary`nRestore Spark ML-compatible metadata part naming for bounded ComplexParams artifacts, align the ModelParam code-generation assertion with PipelineSerializer, and scope the SAR lazy-DataFrame round trip to an explicit trusted session load.

## Prompting Intent`nThe engineer asked to finish the MSRC deserialization remediation as a well-tested pull request, resolve all CI failures, and keep the follow-up narrowly focused instead of adding broad or suite-specific unsafe-deserialization exceptions.

## Linked Sources`n- MSRC incident and remaining-fix assessment: https://portal.microsofticm.com/imp/v5/incidents/details/31000000568481/msrc`n- Original partial remediation: https://github.com/microsoft/SynapseML/pull/2513`n- Follow-up pull request: microsoft#2678

## Rationale`nWriting metadata directly as part-00000 preserves the bounded, session-backed serializer while retaining Spark ML and sparklyr on-disk compatibility; changing individual R suites would only hide the production contract break. The SAR test enables trust only around loading its locally created lazy DataFrame parameters and restores the previous session state, so production remains fail-closed. The remaining Scala change updates a stale assertion rather than altering runtime behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 27, 2026 01:52
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

Resolved the exact-head Azure failures from build 233002476 in 026e043efa without adding suite-wide bypasses:

  • UnitTests misc: updated the stale ModelParam.pyLoadLine assertion to the bounded PipelineSerializer + JavaParams._from_java contract.
  • RTests core / RTests deep-learning: bounded ComplexParams metadata now retains Spark ML's metadata/part-00000 and _SUCCESS layout required by sparklyr. Representative fixtures at all Azure-reported nesting shapes regenerate with that exact path.
  • PythonTests core: the handwritten SAR lazy-DataFrame persistence test now uses the documented session-scoped trusted load and restores the prior Spark setting; production remains fail-closed.

Current-head evidence:

  • JDK 11 / Spark 3.5: 59 focused serializer, metadata, and generated-code tests pass; core main/test Scalastyle passes.
  • JDK 17 / Spark 4.1 / Scala 2.13: the same 59 focused tests pass; core test Scalastyle passes.
  • Generated core and deep-learning R fixtures contain the exact metadata paths missing in Azure.
  • The SAR trusted round trip passes against the freshly built jar and confirms trust is unset after load.
  • Black 22.3.0 and git diff --check pass.

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 45/45 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants