Skip to content

[6.1][Package CMO] Add deserialization checks to ensure correct memory layout #78700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

elsh
Copy link
Contributor

@elsh elsh commented Jan 17, 2025

  • Explanation: Package CMO enables bypassing resilience barrier when accessing a decl, with an assumption that the decl's memory layout is correct. However, when its members are not correctly deserialized, bypassing resilience can lead to accessing an incorrect memory offset, potentially leading to a runtime crash. Currently, members are deserialized lazily and the deserialization errors are silently dropped with an attempt to recover later on, which may not always succeed. The cherry-picked PR loads members if not loaded already and checks for deserialization errors when resilience bypassing is triggered, and emits a diagnostic if an error is found, thus preventing a direct access to the wrong memory layout.
  • Scope: Modules that import modules with Package CMO enabled.
  • Issues: rdar://132411524
  • Original PRs: Package CMO: add deserialization checks to ensure correct memory layout #78258
  • Risk: Low; a deserialization error is found at build time instead of at runtime, and a flag to opt out is also provided in the PR.
  • Testing: unit tests added.
  • Reviewers: @aschwaighofer

…e memory layout of the

decl being accessed is correct. When this assumption fails due to a deserialization error
of its members, the use site accesses the layout with a wrong field offset, resulting in
UB or a crash. The deserialization error is currently not caught at compile time due to
LangOpts.EnableDeserializationRecovery being enabled by default to allow for recovery of some
of the deserialization errors at a later time. In case of member deserialization, however,
it's not necessarily recovered later on.

This PR tracks whether member deserialization had an error by recursively loading members and
checking for deserialization error, and fails and emits a diagnostic. It prevents resilience
bypassing when the deserialized decl's layout is incorrect. It also provides an opt-out flag
for deserialization checks for temporary migration purposes.

Resolves rdar://132411524
@elsh elsh requested a review from a team as a code owner January 17, 2025 00:31
@elsh
Copy link
Contributor Author

elsh commented Jan 17, 2025

@swift-ci test

@elsh elsh requested a review from nkcsgexi January 17, 2025 00:31
Copy link
Contributor

@nkcsgexi nkcsgexi left a comment

Choose a reason for hiding this comment

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

Discussed with Ellie about this offline. This should be a safe change to cherry-pick.

@elsh elsh changed the title Package optimization allows bypassing resilience, but that assumes th… [6.1] [Package CMO] Add deserialization checks to ensure correct memory layout Jan 17, 2025
@elsh elsh changed the title [6.1] [Package CMO] Add deserialization checks to ensure correct memory layout [6.1][Package CMO] Add deserialization checks to ensure correct memory layout Jan 17, 2025
@elsh elsh added the 🍒 release cherry pick Flag: Release branch cherry picks label Jan 17, 2025
@elsh
Copy link
Contributor Author

elsh commented Jan 18, 2025

@swift-ci test windows

1 similar comment
@elsh
Copy link
Contributor Author

elsh commented Jan 20, 2025

@swift-ci test windows

@elsh elsh merged commit ecdbbd5 into release/6.1 Jan 21, 2025
5 checks passed
@elsh elsh deleted the elsh/rel/pcmo-deserialization-diags branch January 21, 2025 02:06
@elsh
Copy link
Contributor Author

elsh commented Jan 30, 2025

Error has been downgraded to warning: #79019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants