Skip to content

Fix two memory corruption bugs when modifying SwiftSyntax nodes #1708

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 19, 2023
Merged

Fix two memory corruption bugs when modifying SwiftSyntax nodes #1708

merged 4 commits into from
Jun 19, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented May 26, 2023 •
edited
Loading

This PR fixes two different 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 an explicit arena parameter to that function to make sure the arena is always alive
  • 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.

I measured performance of swift-format with this change applied and did not see a performance regression when linting swift-syntax.

rdar://109860357

@ahoppen ahoppen requested review from rintaro and bnbarham May 26, 2023 20:15
@ahoppen
Copy link
Member Author

ahoppen commented May 26, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/memory-corruption branch from 54c0a8a to 567d1d8 Compare June 1, 2023 22:27
@ahoppen ahoppen changed the title Fix a memory corruption bug when modifying SwiftSyntax nodes Fix two memory corruption bugs when modifying SwiftSyntax nodes Jun 1, 2023
@ahoppen ahoppen mentioned this pull request Jun 1, 2023
Merged
@ahoppen
Copy link
Member Author

ahoppen commented Jun 1, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jun 1, 2023

@swift-ci Please test Windows

@ahoppen ahoppen force-pushed the ahoppen/memory-corruption branch 2 times, most recently from 059ec6a to de3d08f Compare June 3, 2023 00:50
@ahoppen
Copy link
Member Author

ahoppen commented Jun 3, 2023

@swift-ci Please test

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Jun 3, 2023

@swift-ci Please test

bnbarham
bnbarham reviewed Jun 3, 2023
View reviewed changes
Sources/SwiftSyntax/SyntaxData.swift Outdated
Comment on lines 334 to 337
/// - newChildArena: The arena in which `newChild` resides.
/// - arena: The arena in which the new node will be allocated.
Copy link
Contributor

Choose a reason for hiding this comment

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

All the names no longer match up 😅 newChild is actually with in terms of the API, and newChildArena == rawNodeArena. arena == allocationArena

Copy link
Member Author

Choose a reason for hiding this comment

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

If you generate this comment using Xcode (Cmd-Option-/), it will use the internal parameter names instead of the external ones 🤷🏽

bnbarham
bnbarham reviewed Jun 3, 2023
View reviewed changes
Sources/SwiftSyntax/SyntaxData.swift Outdated
return replacingSelf(newRaw, arena: arena)
func replacingChild(at index: Int, with newChild: RawSyntax?, rawNodeArena: SyntaxArena?, allocationArena: SyntaxArena) -> SyntaxData {
if let newChild {
precondition(newChild.arena === rawNodeArena)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this not just be newChild?.arena == rawNodeArena?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to

precondition(newChild?.arena === rawNodeArena || newChild == nil)

We don’t hit the newChild == nil && rawNodeArena != nil case at the moment AFAICT but there’s nothing really wrong with that, so I wanted to allow it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems odd to me to allow it, they're supposed to be the same so if newChild is nil and rawNodeArena isn't that would suggest something strange is going on.

@ahoppen ahoppen force-pushed the ahoppen/memory-corruption branch from 3bcc618 to 97cd6ed Compare June 3, 2023 02:36
@ahoppen
Copy link
Member Author

ahoppen commented Jun 3, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/memory-corruption branch from c17d2de to 928f967 Compare June 5, 2023 00:46
@ahoppen
Copy link
Member Author

ahoppen commented Jun 5, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/memory-corruption branch from 928f967 to 875a3ad Compare June 12, 2023 21:56
@ahoppen
Copy link
Member Author

ahoppen commented Jun 12, 2023

@swift-ci Please test

@ahoppen ahoppen mentioned this pull request Jun 13, 2023
Closed
@ahoppen
Copy link
Member Author

ahoppen commented Jun 14, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jun 14, 2023

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Jun 15, 2023

@swift-ci Please test Windows

@ahoppen ahoppen force-pushed the ahoppen/memory-corruption branch from 3b0e9a3 to 1357c37 Compare June 16, 2023 13:36
@ahoppen
Copy link
Member Author

ahoppen commented Jun 16, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jun 16, 2023

@swift-ci Please test Windows

bnbarham
bnbarham approved these changes Jun 17, 2023
View reviewed changes
Sources/SwiftSyntax/SyntaxData.swift Outdated
static func forRoot(_ raw: RawSyntax) -> SyntaxData {
SyntaxData(raw, info: .root(.init(arena: raw.arena)))
///
/// `arena` must be the arena in which `raw` is allocated. It is passed to
Copy link
Contributor

Choose a reason for hiding this comment

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

arena is rawNodeArena now 😅

Sources/SwiftSyntax/SyntaxData.swift Outdated
func replacingSelf(_ newRaw: RawSyntax, arena: SyntaxArena) -> SyntaxData {
/// - newRaw: The node that should replace `self`
/// - rawNodeArena: The arena in which `newRaw` resides
/// - arena: The arena in which new nodes should be allocated
Copy link
Contributor

Choose a reason for hiding this comment

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

And this arena is allocationArena

@ahoppen ahoppen force-pushed the ahoppen/memory-corruption branch from 1357c37 to 080b216 Compare June 19, 2023 07:43
@ahoppen
Copy link
Member Author

ahoppen commented Jun 19, 2023

@swift-ci Please test

ahoppen added 4 commits June 19, 2023 12:44
@ahoppen
Add an arena parameter to SyntaxData.forRoot
34f1c33 
The `arena` parameter must be the same one that `raw` is allocated in and ensures that the arena of `raw` doesn’t get deallocated before the `SyntaxData` can retain it.

This removes the need for a few `withExtendedLifetime` calls that we had earlier.
@ahoppen
When replacing a node’s child, make sure the new child doesn’t get pr…
f242e73 
…e-maturely deallocated

Previously, depending on the optimization level, when changing a nodes’s property, it could happen that `value` got de-allocated (because only `value.raw` was being accessed), deallocating the arena that `value.raw` lives in, resulting in freed memory access.
@ahoppen
Split notion of rawNodeArena and allocationArena
7116773 
The `arena` parameter to `replacingSelf` and `replacingChild` served two purposes: To keep the arena of `raw`/`newChild` alive and to specify which arena new nodes should be allocated in. In almost all cases, these areneas coincided but for `SyntaxRewriter.rewrite` they diverged, causing an arena to be added as a child arena after it itself has children, causing a precondition failure.

Split these different purposes of arenas into two different parameters.
@ahoppen
Extend lifetime of children in SyntaxCollection initializer
2441d19 
We were already doing this for layout nodes but we forgot to do it for collection nodes.
@ahoppen ahoppen force-pushed the ahoppen/memory-corruption branch from 080b216 to 2441d19 Compare June 19, 2023 10:44
@ahoppen
Copy link
Member Author

ahoppen commented Jun 19, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge June 19, 2023 10:45
@kimdv
Copy link
Contributor

kimdv commented Jun 19, 2023

@swift-ci please test windows

@ahoppen ahoppen merged commit cbac4b7 into swiftlang:main Jun 19, 2023
@ahoppen ahoppen deleted the ahoppen/memory-corruption branch June 19, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rintaro rintaro rintaro approved these changes

@bnbarham bnbarham bnbarham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ahoppen @kimdv @rintaro @bnbarham