Skip to content

[5.9] Fix two memory corruption bugs in SwiftSyntax #1719

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

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Jun 1, 2023

This is a slightly less risky version of #1708


  • Explanation: This PR fixes three different potential memory corruption bugs when modifying SwiftSyntax nodes
    • SyntaxData.forRoot takes a RawSyntax, which holds a reference to its SyntaxArena but RawSyntax doesn’t retain its arena. We were already taking care of this in multiple places by adding a withExtendedLifetime for the arena around it but missed a case in SyntaxData.replacingSelf. Add withExtendedLifetime here as well.
    • The replacingChild function took a RawSyntax, which again doesn’t retain its arena. When changing any syntax node’s child, we have a value, which is a Syntax node but all we did in the setter was to pass value.raw into replacingChild. This left an opportunity for the optimizer to deallocate the Syntax node (and thus it’s arena) before the call to replacingChild because all we needed from hat node was the RawSyntax. To fix this, add another version of replacingChild that takes a SyntaxData (which does retain the arena) and use that instead.
    • SyntaxArena.rewrite passed a new SyntaxArena to replacingSelf, but replacingSelf expected to be passed the arena in which rewritten was allocated.
  • Scope: Functions that modified syntax nodes
  • Risk: Low, this effectively PR only adds withExtendedLifetime calls
  • Testing: Ran unit tests and verified that swift-format doesn’t regress in performance
  • Issue: rdar://109860357
  • Reviewer:

This PR fixes three different potential memory corruption bugs when modifying SwiftSyntax nodes

- `SyntaxData.forRoot` takes a `RawSyntax`, which holds a reference to its `SyntaxArena` but `RawSyntax` doesn’t retain its arena. We were already taking care of this in multiple places by adding a `withExtendedLifetime` for the arena around it but missed a case in `SyntaxData.replacingSelf`. Add `withExtendedLifetime` here as well.
- The `replacingChild` function took a `RawSyntax`, which again doesn’t retain its arena. When changing any syntax node’s child, we have a `value`, which is a `Syntax` node but all we did in the setter was to pass `value.raw` into `replacingChild`. This left an opportunity for the optimizer to deallocate the `Syntax` node (and thus it’s arena) before the call to `replacingChild` because all we needed from hat node was the `RawSyntax`. To fix this, add another version of `replacingChild` that takes a `SyntaxData` (which does retain the arena) and use that instead.
- `SyntaxArena.rewrite` passed a new `SyntaxArena` to `replacingSelf`, but `replacingSelf` expected to be passed the arena in which `rewritten` was allocated.

rdar://109860357
@ahoppen ahoppen requested review from rintaro and bnbarham June 1, 2023 23:31
@ahoppen
Copy link
Member Author

ahoppen commented Jun 1, 2023

@swift-ci Please test

@ahoppen ahoppen changed the title Fix two memory corruption bugs in SwiftSyntax [5.9] Fix two memory corruption bugs in SwiftSyntax Jun 2, 2023
@ahoppen ahoppen merged commit 165fc6d into swiftlang:release/5.9 Jun 2, 2023
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.

3 participants