Fix two memory corruption bugs when modifying SwiftSyntax nodes #1708
Conversation
|
@swift-ci Please test |
ahoppen
force-pushed
the
ahoppen/memory-corruption
branch
from
54c0a8a to
567d1d8
Compare
ahoppen
changed the title
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
ahoppen
force-pushed
the
ahoppen/memory-corruption
branch
2 times, most recently
from
059ec6a to
de3d08f
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
| /// - newChildArena: The arena in which `newChild` resides. | ||
| /// - arena: The arena in which the new node will be allocated. |
There was a problem hiding this comment.
All the names no longer match up 😅 newChild is actually with in terms of the API, and newChildArena == rawNodeArena. arena == allocationArena
There was a problem hiding this comment.
If you generate this comment using Xcode (Cmd-Option-/), it will use the internal parameter names instead of the external ones 🤷🏽
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) |
There was a problem hiding this comment.
Can this not just be newChild?.arena == rawNodeArena?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
ahoppen/memory-corruption
branch
from
3bcc618 to
97cd6ed
Compare
|
@swift-ci Please test |
ahoppen
force-pushed
the
ahoppen/memory-corruption
branch
from
c17d2de to
928f967
Compare
|
@swift-ci Please test |
ahoppen
force-pushed
the
ahoppen/memory-corruption
branch
from
928f967 to
875a3ad
Compare
|
@swift-ci Please test |
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
1 similar comment
|
@swift-ci Please test Windows |
ahoppen
force-pushed
the
ahoppen/memory-corruption
branch
from
3b0e9a3 to
1357c37
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
And this arena is allocationArena
ahoppen
force-pushed
the
ahoppen/memory-corruption
branch
from
1357c37 to
080b216
Compare
|
@swift-ci Please test |
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.
…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.
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.
We were already doing this for layout nodes but we forgot to do it for collection nodes.
ahoppen
force-pushed
the
ahoppen/memory-corruption
branch
from
080b216 to
2441d19
Compare
|
@swift-ci Please test |
|
@swift-ci please test windows |
4 participants
This PR fixes two different memory corruption bugs when modifying SwiftSyntax nodes
SyntaxData.forRoottakes aRawSyntax, which holds a reference to itsSyntaxArenabutRawSyntaxdoesn’t retain its arena. We were already taking care of this in multiple places by adding awithExtendedLifetimefor the arena around it but missed a case inSyntaxData.replacingSelf. Add an explicitarenaparameter to that function to make sure the arena is always alivereplacingChildfunction took aRawSyntax, which again doesn’t retain its arena. When changing any syntax node’s child, we have avalue, which is aSyntaxnode but all we did in the setter was to passvalue.rawintoreplacingChild. This left an opportunity for the optimizer to deallocate theSyntaxnode (and thus it’s arena) before the call toreplacingChildbecause all we needed from hat node was theRawSyntax. To fix this, add another version ofreplacingChildthat takes aSyntaxData(which does retain the arena) and use that instead.I measured performance of
swift-formatwith this change applied and did not see a performance regression when linting swift-syntax.rdar://109860357