Skip to content

Commit d349802

Browse files
adamzieladamziel
committed
Break native AST wrapper cycles
1 parent ed685e3 commit d349802

4 files changed

Lines changed: 287 additions & 442 deletions

File tree

‎packages/mysql-on-sqlite/src/mysql/native/class-wp-mysql-native-parser-node.php‎

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,16 @@
1010
* `$children` array and behaves like a plain WP_Parser_Node from then on.
1111
*/
1212
class WP_MySQL_Native_Parser_Node extends WP_Parser_Node {
13-
private $native_ast = null;
14-
private $native_node_index = null;
15-
private $was_mutated = false;
13+
private $was_mutated = false;
1614

17-
public function __construct( $rule_id, $rule_name, $native_ast = null, $native_node_index = null ) {
15+
public function __construct( $rule_id, $rule_name ) {
1816
parent::__construct( $rule_id, $rule_name );
17+
}
1918

20-
$this->native_ast = $native_ast;
21-
$this->native_node_index = $native_node_index;
19+
public function __destruct() {
20+
if ( function_exists( 'wp_sqlite_mysql_native_ast_release_wrapper' ) ) {
21+
wp_sqlite_mysql_native_ast_release_wrapper( $this );
22+
}
2223
}
2324

2425
/** @inheritDoc */
@@ -41,127 +42,127 @@ public function has_child(): bool {
4142
if ( $this->was_mutated() ) {
4243
return parent::has_child();
4344
}
44-
return wp_sqlite_mysql_native_ast_has_child( $this->native_ast, $this->native_node_index );
45+
return wp_sqlite_mysql_native_ast_has_child( $this );
4546
}
4647

4748
/** @inheritDoc */
4849
public function has_child_node( ?string $rule_name = null ): bool {
4950
if ( $this->was_mutated() ) {
5051
return parent::has_child_node( $rule_name );
5152
}
52-
return wp_sqlite_mysql_native_ast_has_child_node( $this->native_ast, $this->native_node_index, $rule_name );
53+
return wp_sqlite_mysql_native_ast_has_child_node( $this, $rule_name );
5354
}
5455

5556
/** @inheritDoc */
5657
public function has_child_token( ?int $token_id = null ): bool {
5758
if ( $this->was_mutated() ) {
5859
return parent::has_child_token( $token_id );
5960
}
60-
return wp_sqlite_mysql_native_ast_has_child_token( $this->native_ast, $this->native_node_index, $token_id );
61+
return wp_sqlite_mysql_native_ast_has_child_token( $this, $token_id );
6162
}
6263

6364
/** @inheritDoc */
6465
public function get_first_child() {
6566
if ( $this->was_mutated() ) {
6667
return parent::get_first_child();
6768
}
68-
return wp_sqlite_mysql_native_ast_get_first_child( $this->native_ast, $this->native_node_index );
69+
return wp_sqlite_mysql_native_ast_get_first_child( $this );
6970
}
7071

7172
/** @inheritDoc */
7273
public function get_first_child_node( ?string $rule_name = null ): ?WP_Parser_Node {
7374
if ( $this->was_mutated() ) {
7475
return parent::get_first_child_node( $rule_name );
7576
}
76-
return wp_sqlite_mysql_native_ast_get_first_child_node( $this->native_ast, $this->native_node_index, $rule_name );
77+
return wp_sqlite_mysql_native_ast_get_first_child_node( $this, $rule_name );
7778
}
7879

7980
/** @inheritDoc */
8081
public function get_first_child_token( ?int $token_id = null ): ?WP_Parser_Token {
8182
if ( $this->was_mutated() ) {
8283
return parent::get_first_child_token( $token_id );
8384
}
84-
return wp_sqlite_mysql_native_ast_get_first_child_token( $this->native_ast, $this->native_node_index, $token_id );
85+
return wp_sqlite_mysql_native_ast_get_first_child_token( $this, $token_id );
8586
}
8687

8788
/** @inheritDoc */
8889
public function get_first_descendant_node( ?string $rule_name = null ): ?WP_Parser_Node {
8990
if ( $this->was_mutated() ) {
9091
return parent::get_first_descendant_node( $rule_name );
9192
}
92-
return wp_sqlite_mysql_native_ast_get_first_descendant_node( $this->native_ast, $this->native_node_index, $rule_name );
93+
return wp_sqlite_mysql_native_ast_get_first_descendant_node( $this, $rule_name );
9394
}
9495

9596
/** @inheritDoc */
9697
public function get_first_descendant_token( ?int $token_id = null ): ?WP_Parser_Token {
9798
if ( $this->was_mutated() ) {
9899
return parent::get_first_descendant_token( $token_id );
99100
}
100-
return wp_sqlite_mysql_native_ast_get_first_descendant_token( $this->native_ast, $this->native_node_index, $token_id );
101+
return wp_sqlite_mysql_native_ast_get_first_descendant_token( $this, $token_id );
101102
}
102103

103104
/** @inheritDoc */
104105
public function get_children(): array {
105106
if ( $this->was_mutated() ) {
106107
return parent::get_children();
107108
}
108-
return wp_sqlite_mysql_native_ast_get_children( $this->native_ast, $this->native_node_index );
109+
return wp_sqlite_mysql_native_ast_get_children( $this );
109110
}
110111

111112
/** @inheritDoc */
112113
public function get_child_nodes( ?string $rule_name = null ): array {
113114
if ( $this->was_mutated() ) {
114115
return parent::get_child_nodes( $rule_name );
115116
}
116-
return wp_sqlite_mysql_native_ast_get_child_nodes( $this->native_ast, $this->native_node_index, $rule_name );
117+
return wp_sqlite_mysql_native_ast_get_child_nodes( $this, $rule_name );
117118
}
118119

119120
/** @inheritDoc */
120121
public function get_child_tokens( ?int $token_id = null ): array {
121122
if ( $this->was_mutated() ) {
122123
return parent::get_child_tokens( $token_id );
123124
}
124-
return wp_sqlite_mysql_native_ast_get_child_tokens( $this->native_ast, $this->native_node_index, $token_id );
125+
return wp_sqlite_mysql_native_ast_get_child_tokens( $this, $token_id );
125126
}
126127

127128
/** @inheritDoc */
128129
public function get_descendants(): array {
129130
if ( $this->was_mutated() ) {
130131
return parent::get_descendants();
131132
}
132-
return wp_sqlite_mysql_native_ast_get_descendants( $this->native_ast, $this->native_node_index );
133+
return wp_sqlite_mysql_native_ast_get_descendants( $this );
133134
}
134135

135136
/** @inheritDoc */
136137
public function get_descendant_nodes( ?string $rule_name = null ): array {
137138
if ( $this->was_mutated() ) {
138139
return parent::get_descendant_nodes( $rule_name );
139140
}
140-
return wp_sqlite_mysql_native_ast_get_descendant_nodes( $this->native_ast, $this->native_node_index, $rule_name );
141+
return wp_sqlite_mysql_native_ast_get_descendant_nodes( $this, $rule_name );
141142
}
142143

143144
/** @inheritDoc */
144145
public function get_descendant_tokens( ?int $token_id = null ): array {
145146
if ( $this->was_mutated() ) {
146147
return parent::get_descendant_tokens( $token_id );
147148
}
148-
return wp_sqlite_mysql_native_ast_get_descendant_tokens( $this->native_ast, $this->native_node_index, $token_id );
149+
return wp_sqlite_mysql_native_ast_get_descendant_tokens( $this, $token_id );
149150
}
150151

151152
/** @inheritDoc */
152153
public function get_start(): int {
153154
if ( $this->was_mutated() ) {
154155
return parent::get_start();
155156
}
156-
return wp_sqlite_mysql_native_ast_get_start( $this->native_ast, $this->native_node_index );
157+
return wp_sqlite_mysql_native_ast_get_start( $this );
157158
}
158159

159160
/** @inheritDoc */
160161
public function get_length(): int {
161162
if ( $this->was_mutated() ) {
162163
return parent::get_length();
163164
}
164-
return wp_sqlite_mysql_native_ast_get_length( $this->native_ast, $this->native_node_index );
165+
return wp_sqlite_mysql_native_ast_get_length( $this );
165166
}
166167

167168
private function was_mutated(): bool {
@@ -173,9 +174,10 @@ private function materialize_native_children(): void {
173174
return;
174175
}
175176

176-
$this->children = wp_sqlite_mysql_native_ast_get_children( $this->native_ast, $this->native_node_index );
177-
$this->native_ast = null;
178-
$this->native_node_index = null;
179-
$this->was_mutated = true;
177+
$this->children = wp_sqlite_mysql_native_ast_get_children( $this );
178+
$this->was_mutated = true;
179+
if ( function_exists( 'wp_sqlite_mysql_native_ast_materialize_wrapper' ) ) {
180+
wp_sqlite_mysql_native_ast_materialize_wrapper( $this );
181+
}
180182
}
181183
}

‎packages/mysql-on-sqlite/tests/mysql/native/WP_MySQL_Native_Parser_Node_Cycle_Tests.php‎

Lines changed: 21 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,19 @@
33
use PHPUnit\Framework\TestCase;
44

55
/**
6-
* Cycle-collection / memory-bound tests for the Rust-side identity cache.
6+
* Memory-bound tests for the Rust-side native wrapper registry.
77
*
8-
* The Rust extension stores cached wrappers in a HashMap. Each cached
9-
* wrapper has a `$native_ast` property pointing back at the AST, forming
10-
* a cycle the cycle collector can only reclaim when the native AST's
11-
* `get_gc` handler exposes the cached wrappers to PHP's collector. These
12-
* tests break in every direction that cycle handling can regress:
8+
* The Rust extension stores AST state in a registry keyed by live PHP wrapper
9+
* pointers. Cached wrappers are raw pointers, not strong PHP references, so
10+
* wrappers must not pin entire ASTs after PHP drops them. These tests break in
11+
* every direction that lifetime handling can regress:
1312
*
1413
* - Loops parsing many ASTs without explicit GC must not grow without
1514
* bound (ordinary mode of use).
16-
* - Walking, mutating, and dropping an AST must reclaim the wrapper
17-
* memory once `gc_collect_cycles()` runs.
15+
* - Walking, mutating, and dropping an AST must reclaim the wrapper memory.
1816
* - Holding a child wrapper after the parent AST goes out of scope must
1917
* not crash, must not corrupt memory, and the AST must stay alive as
20-
* long as that child is reachable.
18+
* long as that child is reachable through the registry.
2119
* - Nested ASTs with overlapping lifetimes must not interfere — dropping
2220
* one mustn't free another's cached wrappers.
2321
* - Mutating a cached wrapper before dropping the AST must still allow
@@ -55,9 +53,8 @@ private function parse( string $sql ): WP_Parser_Node {
5553
* Hostile loop: parse and walk many ASTs in a tight loop, only
5654
* `gc_collect_cycles()` between iterations. Memory must plateau.
5755
*
58-
* Without a working gc_handler the Rust cache retains every AST's
59-
* wrappers forever — peak memory grows linearly with iteration count.
60-
* With the handler, each dropped AST's cycle is collected and the
56+
* If wrapper registry entries or cache pointers are not released, peak
57+
* memory grows linearly with iteration count. With cleanup in place, the
6158
* working set stays bounded.
6259
*/
6360
public function test_repeated_parse_walk_drop_does_not_leak(): void {
@@ -131,9 +128,9 @@ public function test_drop_then_gc_reclaims_cached_wrappers(): void {
131128

132129
/**
133130
* Holding a child wrapper *outlives* the variable holding the root.
134-
* The cache pinning the child must keep the AST alive (no UAF when
135-
* the bridge is called on the orphaned child). Once the child is
136-
* also dropped, GC must collect the whole graph.
131+
* The child's registry entry must keep the AST alive (no UAF when the
132+
* bridge is called on the orphaned child). Once the child is also dropped,
133+
* the registry entry must be released.
137134
*/
138135
public function test_orphaned_child_keeps_ast_alive_then_collects(): void {
139136
$sql = 'SELECT a, b, c FROM t WHERE a + b * c IN (1, 2, 3)';
@@ -142,9 +139,9 @@ public function test_orphaned_child_keeps_ast_alive_then_collects(): void {
142139
return $ast->get_first_descendant_node();
143140
} )();
144141

145-
// Root variable is gone; only the child reference remains, but
146-
// the cache still pins the AST through the back-reference. The
147-
// child must still be functional — accessing it must not crash.
142+
// Root variable is gone; only the child reference remains, but the
143+
// registry entry still pins the AST. The child must still be
144+
// functional — accessing it must not crash.
148145
$this->assertNotNull( $child );
149146
$this->assertIsString( $child->rule_name );
150147
// The child's own children should also resolve without UAF.
@@ -155,16 +152,16 @@ public function test_orphaned_child_keeps_ast_alive_then_collects(): void {
155152
$child = null;
156153
$grand = null;
157154
gc_collect_cycles();
158-
// If the cycle collected, this assertion always passes; the real
159-
// signal is the absence of a segfault during teardown.
155+
// If the registry entry was released, this assertion always passes;
156+
// the real signal is the absence of a segfault during teardown.
160157
$this->addToAssertionCount( 1 );
161158
}
162159

163160
/**
164161
* Mutating a cached wrapper through `append_child` before dropping
165162
* the AST must not block collection. The mutated wrapper's
166-
* `$children` array now contains a non-cached node; that's an extra
167-
* edge for the gc_handler to traverse, not a reason to leak.
163+
* `$children` array now contains a non-cached node; that must not keep
164+
* stale registry/cache entries alive.
168165
*/
169166
public function test_mutation_before_drop_does_not_block_collection(): void {
170167
$sql = 'SELECT 1 + 2';
@@ -230,9 +227,8 @@ public function test_overlapping_asts_do_not_corrupt_each_other(): void {
230227

231228
/**
232229
* Re-walk + drop + collect across many iterations. This is the
233-
* "translator pass on each query" shape of real workloads. The Rust
234-
* cache should give us the perf win of `rewalk` without the memory
235-
* cliff that a missing gc_handler creates.
230+
* "translator pass on each query" shape of real workloads. The wrapper
231+
* registry and cache must not create a memory cliff under repeated walks.
236232
*/
237233
public function test_rewalk_loop_stays_bounded(): void {
238234
$sql = 'SELECT a, b, c, d, e FROM t WHERE (a + b) * (c - d) > e AND f IN (1,2,3,4,5)';

‎packages/mysql-on-sqlite/tests/mysql/native/WP_MySQL_Native_Parser_Node_Identity_Tests.php‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@
77
*
88
* The native extension constructs a fresh PHP wrapper for every accessor
99
* call. Without interning, two reads of the same logical node would yield
10-
* distinct objects, and a mutation made through the first wrapper would
11-
* be invisible through the second. WP_Parser_Node exposes public mutators
12-
* and stable child identity, so the native wrapper must preserve both.
10+
* distinct objects, and a mutation made through a still-live wrapper would be
11+
* invisible through the second. WP_Parser_Node exposes public mutators and
12+
* stable child identity, so the native wrapper must preserve both.
1313
*
1414
* Skipped when the native extension is not loaded — the pure-PHP code
1515
* path already has stable identity by construction.
@@ -47,6 +47,15 @@ public function test_get_first_child_node_returns_same_instance(): void {
4747
$this->assertSame( $first, $second );
4848
}
4949

50+
public function test_native_wrapper_does_not_store_native_ast_handle(): void {
51+
$tree = $this->parse( 'SELECT 1 + 2' );
52+
53+
$reflection = new ReflectionObject( $tree );
54+
55+
$this->assertFalse( $reflection->hasProperty( 'native_ast' ) );
56+
$this->assertFalse( $reflection->hasProperty( 'native_node_index' ) );
57+
}
58+
5059
public function test_get_children_returns_same_instances_across_calls(): void {
5160
$tree = $this->parse( 'SELECT 1, 2, 3' );
5261

@@ -99,6 +108,23 @@ public function test_mutation_on_child_survives_re_read(): void {
99108
$this->assertSame( 'mutated-rule', $same_child->rule_name );
100109
}
101110

111+
public function test_materialized_child_survives_re_read_from_native_parent(): void {
112+
$tree = $this->parse( 'SELECT 1 + 2' );
113+
114+
$child = $tree->get_first_child_node();
115+
$this->assertNotNull( $child );
116+
117+
$synthetic = new WP_Parser_Node( 0, 'synthetic' );
118+
$child->append_child( $synthetic );
119+
120+
$same_child = $tree->get_first_child_node();
121+
$this->assertSame( $child, $same_child );
122+
$this->assertTrue(
123+
in_array( $synthetic, $same_child->get_children(), true ),
124+
'Materialized live child wrappers must stay discoverable through the parent native cache.'
125+
);
126+
}
127+
102128
public function test_mutation_survives_parent_materialization(): void {
103129
$tree = $this->parse( 'SELECT 1 + 2' );
104130

0 commit comments

Comments
 (0)