diff --git a/CHANGELOG.md b/CHANGELOG.md index b31f875751..57dcff7029 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to elephc, a PHP-to-native compiler written in Rust. Releases are listed newest first. ## [Unreleased] +- Fixed a heap corruption when an array grown inside a loop is promoted to mixed-element storage (issue #452): with `$vals[] = 1; $vals[] = 2.0;` in a loop body, the earlier push site was typed and lowered against the pre-promotion element type, so from the second iteration it wrote an unboxed scalar into boxed-cell storage — reading that element produced garbage or a segfault. Loop bodies now pre-scan their push sites and fix the array's element type to `mixed` up front (checker and EIR lowering), materializing the promotion once before the loop so every push boxes correctly. Also fixes the silent value corruption and per-conversion leak of the string/float flavour of the same pattern. - Fixed a constant-propagation miscompile with by-reference calls in a `match` subject (issue #384): `echo match(bump($i)) { ... } . "|" . $i` kept the pre-call constant for `$i` and printed the stale value, because a call's unknown write set was treated as "no writes" instead of forcing conservative invalidation. A read sequenced after any by-reference-mutating call in the same expression now observes the post-call value, matching PHP; pure calls (`gettype`, `strlen`, …) keep their operands foldable. - Fixed PHP parser/codegen parity for parenthesis-free object instantiation (issue #371): `new Foo`, `new self`, `new static`, `new parent`, and `new $class` now compile with empty constructor arguments, while immediate postfix forms such as `new Foo->bar`, `new Foo::bar()`, `new Foo?->bar`, and `new Foo[0]` are rejected instead of being misparsed. - Fixed three PHP parity regressions in the EIR backend: indexed array elements such as `$a[0]` can now be passed to by-reference parameters with copy-on-write storage split before mutation (issue #360); `IteratorAggregate::getIterator()` may declare the marker `Traversable` return type while `foreach` still dispatches through the returned object's concrete `Iterator` methods (issue #385); and catchable private/protected method access plus readonly-property write errors now preserve PHP's receiver/RHS evaluation order before throwing `Error` (issue #383). The merge also removes a duplicate `_spl_error_class_id` data symbol that could make post-merge user assembly fail to assemble. diff --git a/src/ir_lower/stmt/mod.rs b/src/ir_lower/stmt/mod.rs index bef17b5bba..c02582152d 100644 --- a/src/ir_lower/stmt/mod.rs +++ b/src/ir_lower/stmt/mod.rs @@ -446,7 +446,75 @@ fn lower_ifdef( } /// Lowers a `while` loop. +/// Widens locals whose indexed-array element type joins to `mixed` across the loop body's +/// push sites (issue #452) and materializes the promotion once before the loop. Loop bodies +/// are lowered in a single pass, so without this an early `$a[] = ` site is emitted +/// as a raw push against the pre-promotion element type even though the back edge brings the +/// promoted `array` around, writing an unboxed scalar into boxed-cell storage on +/// iterations >= 2. Converting the array up front (in place, same pointer) and fixing its +/// type to `array` makes every push site box its value. `overrides` supplies types +/// for names bound by the loop itself (the `foreach` value/key variables) so a push of the +/// loop variable joins with its real element type. Locals without a materialized slot yet +/// (first assigned inside the body, hence fresh every iteration) are left untouched. +fn widen_loop_grown_arrays( + ctx: &mut LoweringContext<'_, '_>, + body: &[Stmt], + update: Option<&Stmt>, + overrides: &[(&str, PhpType)], + span: Option, +) { + let names = { + let lookup = |name: &str| -> Option { + if let Some((_, ty)) = overrides.iter().find(|(n, _)| *n == name) { + return Some(ty.clone()); + } + // A name with no declared slot yet (first assigned inside the loop body) is + // genuinely unknown at loop entry: report it as such instead of the Mixed + // fallback `local_type` would return, so the prescan does not take Mixed as + // widening evidence for it (mirrors the checker's `env.get` lookup). + if !ctx.local_slots.contains_key(name) { + return None; + } + Some(ctx.local_type(name)) + }; + crate::types::checker::loop_grown_mixed_array_pushes(body, update, &lookup) + }; + for name in names { + if !ctx.local_slots.contains_key(&name) { + continue; + } + let array_value = ctx.load_local(&name, span); + if array_value.ir_type != IrType::Heap(crate::ir::IrHeapKind::Array) { + continue; + } + let mixed_array_ty = PhpType::Array(Box::new(PhpType::Mixed)); + let converted = ctx.emit_value( + Op::ArrayToMixed, + vec![array_value.value], + None, + mixed_array_ty.clone(), + Op::ArrayToMixed.default_effects(), + span, + ); + ctx.store_mutated_local(&name, converted, mixed_array_ty, span); + } +} + +/// Mirrors the checker's `foreach` value binding for the loop-widening prescan: the element +/// type for an indexed/associative array source variable, `mixed` otherwise. +fn foreach_prescan_value_type(ctx: &LoweringContext<'_, '_>, array: &Expr) -> PhpType { + let ExprKind::Variable(name) = &array.kind else { + return PhpType::Mixed; + }; + match ctx.local_type(name) { + PhpType::Array(elem) => *elem, + PhpType::AssocArray { value, .. } => *value, + _ => PhpType::Mixed, + } +} + fn lower_while(ctx: &mut LoweringContext<'_, '_>, condition: &Expr, body: &[Stmt]) { + widen_loop_grown_arrays(ctx, body, None, &[], Some(condition.span)); let header = ctx.builder.create_named_block("while.cond", Vec::new()); let body_block = ctx.builder.create_named_block("while.body", Vec::new()); let exit = ctx.builder.create_named_block("while.exit", Vec::new()); @@ -479,6 +547,7 @@ fn lower_while(ctx: &mut LoweringContext<'_, '_>, condition: &Expr, body: &[Stmt /// Lowers a `do while` loop. fn lower_do_while(ctx: &mut LoweringContext<'_, '_>, body: &[Stmt], condition: &Expr) { + widen_loop_grown_arrays(ctx, body, None, &[], Some(condition.span)); let body_block = ctx.builder.create_named_block("do.body", Vec::new()); let cond_block = ctx.builder.create_named_block("do.cond", Vec::new()); let exit = ctx.builder.create_named_block("do.exit", Vec::new()); @@ -523,6 +592,10 @@ fn lower_for( if ctx.builder.insertion_block_is_terminated() { return; } + let widen_span = condition + .map(|c| c.span) + .or_else(|| body.first().map(|s| s.span)); + widen_loop_grown_arrays(ctx, body, update, &[], widen_span); let header = ctx.builder.create_named_block("for.cond", Vec::new()); let body_block = ctx.builder.create_named_block("for.body", Vec::new()); @@ -1037,6 +1110,15 @@ fn lower_foreach( value_by_ref: bool, body: &[Stmt], ) { + // Widen before the source is lowered so an iterated-and-pushed array is loaded with + // its fixed-point element type, and bind the loop variables' prescan types so a push + // of the foreach value joins with its real element type. + let prescan_value_ty = foreach_prescan_value_type(ctx, array); + let mut overrides: Vec<(&str, PhpType)> = vec![(value_var, prescan_value_ty)]; + if let Some(key_var) = key_var { + overrides.push((key_var, PhpType::Mixed)); + } + widen_loop_grown_arrays(ctx, body, None, &overrides, Some(array.span)); let source = lower_expr(ctx, array); let source_php_ty = ctx.builder.value_php_type(source.value); let source_ty = source_php_ty.codegen_repr(); diff --git a/src/types/checker/loop_widening.rs b/src/types/checker/loop_widening.rs new file mode 100644 index 0000000000..ce82eade36 --- /dev/null +++ b/src/types/checker/loop_widening.rs @@ -0,0 +1,338 @@ +//! Purpose: +//! Pre-scans a loop body for `$array[] = value` pushes whose element types widen a +//! local indexed array to `mixed` across the loop back-edge (issue #452), so both the +//! type checker and EIR lowering can fix the array's element type to `mixed` *before* +//! processing the body once. +//! +//! Called from: +//! - `crate::types::checker::stmt_check::control_flow` (loop arms widen the `TypeEnv`). +//! - `crate::ir_lower::stmt` (loop lowering widens `local_types` and materializes the +//! promotion before emitting the body). +//! +//! Key details: +//! - Both passes are single-pass over loop bodies; without this scan an early push site +//! is typed/lowered against the pre-promotion element type and writes an unboxed +//! scalar into mixed-element storage on iterations >= 2, corrupting the heap. +//! - Only the widening-to-`mixed` transition is reported: it is the one that changes the +//! element representation (raw scalar slots vs boxed cells). Same-type pushes and +//! `never -> T` growth keep their current lowering. +//! - Value types resolve syntactically (`infer_expr_type_syntactic`); variables resolve +//! through the caller-supplied lookup (loop-entry types, e.g. a `foreach` value +//! variable's real element type) joined with self-evident literal assignments found +//! inside the loop body (`$x = 1; $a[] = $x;`). +//! - A pushed value with no usable evidence — a variable defined only inside the loop +//! from non-literal sources — contributes nothing to the join. Treating it as `mixed` +//! would spuriously widen same-typed rebuild loops (e.g. the synthesized +//! `MultipleIterator::detachIterator` body, whose `array` rebuild must stay +//! assignable to its typed property); genuinely-widening pushes of such values are +//! still promoted per-site by the existing lowering. + +use crate::parser::ast::{Expr, ExprKind, Stmt, StmtKind}; +use crate::types::checker::infer_expr_type_syntactic; +use crate::types::PhpType; + +/// Returns the names of locals that currently hold a non-`mixed` indexed array and whose +/// element type joins to `mixed` across the `$name[] = value` pushes found in the loop +/// body (and the optional `for` update statement). `lookup` supplies the current type of +/// a local at loop entry; names it does not know are skipped as push targets. A pushed +/// value with no usable type evidence — a variable defined only inside the loop from a +/// non-literal source, such as `$candidate = $this->iterators[$i]` — contributes nothing +/// to the join instead of forcing `mixed`, so same-typed rebuild loops (common in +/// compiler-synthesized SPL method bodies) are not spuriously widened. +pub fn loop_grown_mixed_array_pushes( + body: &[Stmt], + update: Option<&Stmt>, + lookup: &dyn Fn(&str) -> Option, +) -> Vec { + let mut pushes: Vec<(&str, &Expr)> = Vec::new(); + collect_array_pushes(body, &mut pushes); + if let Some(stmt) = update { + collect_array_push_stmt(stmt, &mut pushes); + } + let mut literal_assignments: Vec<(&str, Option)> = Vec::new(); + collect_literal_assignments(body, &mut literal_assignments); + if let Some(stmt) = update { + collect_literal_assignment_stmt(stmt, &mut literal_assignments); + } + let mut names: Vec = Vec::new(); + for (name, _) in &pushes { + if names.iter().any(|n| n == name) { + continue; + } + let Some(PhpType::Array(elem)) = lookup(name) else { + continue; + }; + if *elem == PhpType::Mixed { + continue; + } + let joined = pushes + .iter() + .filter(|(n, _)| n == name) + .fold(*elem, |acc, (_, value)| { + match resolve_pushed_value_type(value, lookup, &literal_assignments) { + Some(pushed) => join_pushed_element_type(acc, pushed), + None => acc, + } + }); + if joined == PhpType::Mixed { + names.push(name.to_string()); + } + } + names +} + +/// Resolves the static type a pushed value contributes to the element join, or `None` +/// when there is no usable evidence. A pushed variable joins its loop-entry type with any +/// self-evident literal assignments to it inside the loop body (`$x = 1; $a[] = $x;`); +/// a variable that is only ever assigned from non-literal sources inside the loop yields +/// no evidence — the loop-entry lookup is authoritative and unknown stays unknown, never +/// `mixed`. Non-variable values use the shared syntactic inference as before. +fn resolve_pushed_value_type( + value: &Expr, + lookup: &dyn Fn(&str) -> Option, + literal_assignments: &[(&str, Option)], +) -> Option { + match &value.kind { + ExprKind::Variable(name) => { + let entry = lookup(name); + let body = joined_literal_assignment_type(name, literal_assignments); + match (entry, body) { + (Some(a), Some(b)) => Some(join_pushed_element_type(a, b)), + (Some(a), None) => Some(a), + (None, Some(b)) => Some(b), + (None, None) => None, + } + } + _ => Some(infer_expr_type_syntactic(value)), + } +} + +/// Joins the recorded in-loop literal assignments for `name`. Returns `None` when the +/// variable has no literal assignment or any of its assignments is non-literal (a poison +/// entry) — partial literal evidence must not overrule the unknown non-literal sources. +fn joined_literal_assignment_type( + name: &str, + literal_assignments: &[(&str, Option)], +) -> Option { + let mut joined: Option = None; + for (candidate, ty) in literal_assignments { + if *candidate != name { + continue; + } + let Some(ty) = ty else { + return None; + }; + joined = Some(match joined { + Some(acc) => join_pushed_element_type(acc, ty.clone()), + None => ty.clone(), + }); + } + joined +} + +/// Returns the self-evident scalar type of a literal expression, or `None` for anything +/// that needs real inference (the shared syntactic helper defaults unknown constructs to +/// `Int`, which is not usable as widening evidence). +fn literal_expr_type(value: &Expr) -> Option { + match &value.kind { + ExprKind::IntLiteral(_) => Some(PhpType::Int), + ExprKind::FloatLiteral(_) => Some(PhpType::Float), + ExprKind::StringLiteral(_) => Some(PhpType::Str), + ExprKind::BoolLiteral(_) => Some(PhpType::Bool), + _ => None, + } +} + +/// Collects `$name = ` assignments from every statement in `stmts`, recursively, +/// recording a poison entry (`None` type) for non-literal assignments so partial evidence +/// never overrules an unknown source. +fn collect_literal_assignments<'a>(stmts: &'a [Stmt], out: &mut Vec<(&'a str, Option)>) { + for stmt in stmts { + collect_literal_assignment_stmt(stmt, out); + } +} + +/// Collects literal assignments from one statement, mirroring the nested-statement +/// recursion of `collect_array_push_stmt` (only bodies that execute within the loop +/// iteration are descended into). +fn collect_literal_assignment_stmt<'a>(stmt: &'a Stmt, out: &mut Vec<(&'a str, Option)>) { + match &stmt.kind { + StmtKind::Assign { name, value } | StmtKind::TypedAssign { name, value, .. } => { + out.push((name.as_str(), literal_expr_type(value))); + } + StmtKind::If { + then_body, + elseif_clauses, + else_body, + .. + } => { + collect_literal_assignments(then_body, out); + for (_, clause_body) in elseif_clauses { + collect_literal_assignments(clause_body, out); + } + if let Some(else_body) = else_body { + collect_literal_assignments(else_body, out); + } + } + StmtKind::IfDef { + then_body, + else_body, + .. + } => { + collect_literal_assignments(then_body, out); + if let Some(else_body) = else_body { + collect_literal_assignments(else_body, out); + } + } + StmtKind::While { body, .. } + | StmtKind::DoWhile { body, .. } + | StmtKind::IncludeOnceGuard { body, .. } => collect_literal_assignments(body, out), + StmtKind::Foreach { + key_var, + value_var, + body, + .. + } => { + // The foreach bindings assign non-literal values each iteration: poison them. + if let Some(key_var) = key_var { + out.push((key_var.as_str(), None)); + } + out.push((value_var.as_str(), None)); + collect_literal_assignments(body, out); + } + StmtKind::For { + init, + update, + body, + .. + } => { + if let Some(init) = init { + collect_literal_assignment_stmt(init, out); + } + if let Some(update) = update { + collect_literal_assignment_stmt(update, out); + } + collect_literal_assignments(body, out); + } + StmtKind::Switch { cases, default, .. } => { + for (_, case_body) in cases { + collect_literal_assignments(case_body, out); + } + if let Some(default) = default { + collect_literal_assignments(default, out); + } + } + StmtKind::Try { + try_body, + catches, + finally_body, + } => { + collect_literal_assignments(try_body, out); + for catch in catches { + collect_literal_assignments(&catch.body, out); + } + if let Some(finally_body) = finally_body { + collect_literal_assignments(finally_body, out); + } + } + StmtKind::Synthetic(stmts) => collect_literal_assignments(stmts, out), + _ => {} + } +} + +/// Joins two indexed-array element types on the widening lattice used by push sites: +/// equal types stay, `never` adopts the other side, and any other combination widens to +/// `mixed` (the representation-changing transition this scan exists to detect). +fn join_pushed_element_type(a: PhpType, b: PhpType) -> PhpType { + if a == b { + a + } else if a == PhpType::Never { + b + } else if b == PhpType::Never { + a + } else { + PhpType::Mixed + } +} + +/// Collects `$name[] = value` pushes from every statement in `stmts`, recursively. +fn collect_array_pushes<'a>(stmts: &'a [Stmt], out: &mut Vec<(&'a str, &'a Expr)>) { + for stmt in stmts { + collect_array_push_stmt(stmt, out); + } +} + +/// Collects pushes from one statement, recursing into every nested statement body that +/// executes as part of the enclosing loop iteration. Declaration bodies (functions, +/// classes) do not execute in the loop and closures capture by value by default, so +/// neither is descended into. +fn collect_array_push_stmt<'a>(stmt: &'a Stmt, out: &mut Vec<(&'a str, &'a Expr)>) { + match &stmt.kind { + StmtKind::ArrayPush { array, value } => out.push((array.as_str(), value)), + StmtKind::If { + then_body, + elseif_clauses, + else_body, + .. + } => { + collect_array_pushes(then_body, out); + for (_, clause_body) in elseif_clauses { + collect_array_pushes(clause_body, out); + } + if let Some(else_body) = else_body { + collect_array_pushes(else_body, out); + } + } + StmtKind::IfDef { + then_body, + else_body, + .. + } => { + collect_array_pushes(then_body, out); + if let Some(else_body) = else_body { + collect_array_pushes(else_body, out); + } + } + StmtKind::While { body, .. } + | StmtKind::DoWhile { body, .. } + | StmtKind::Foreach { body, .. } + | StmtKind::IncludeOnceGuard { body, .. } => collect_array_pushes(body, out), + StmtKind::For { + init, + update, + body, + .. + } => { + if let Some(init) = init { + collect_array_push_stmt(init, out); + } + if let Some(update) = update { + collect_array_push_stmt(update, out); + } + collect_array_pushes(body, out); + } + StmtKind::Switch { cases, default, .. } => { + for (_, case_body) in cases { + collect_array_pushes(case_body, out); + } + if let Some(default) = default { + collect_array_pushes(default, out); + } + } + StmtKind::Try { + try_body, + catches, + finally_body, + } => { + collect_array_pushes(try_body, out); + for catch in catches { + collect_array_pushes(&catch.body, out); + } + if let Some(finally_body) = finally_body { + collect_array_pushes(finally_body, out); + } + } + StmtKind::Synthetic(stmts) => collect_array_pushes(stmts, out), + _ => {} + } +} diff --git a/src/types/checker/mod.rs b/src/types/checker/mod.rs index 081d95ced8..4faadddb4f 100644 --- a/src/types/checker/mod.rs +++ b/src/types/checker/mod.rs @@ -26,6 +26,7 @@ mod driver; mod extern_decl; mod functions; mod inference; +mod loop_widening; mod method_pass; mod schema; mod stmt_check; @@ -45,6 +46,7 @@ use crate::types::{ }; pub use inference::{infer_expr_type_syntactic, infer_return_type_syntactic}; +pub(crate) use loop_widening::loop_grown_mixed_array_pushes; pub(crate) use inference::closure_body_uses_this; pub(crate) use builtin_types::InterfaceDeclInfo; use builtin_types::validate_magic_method_contracts; diff --git a/src/types/checker/stmt_check/control_flow.rs b/src/types/checker/stmt_check/control_flow.rs index 187bcc9182..70b602c7ff 100644 --- a/src/types/checker/stmt_check/control_flow.rs +++ b/src/types/checker/stmt_check/control_flow.rs @@ -19,6 +19,21 @@ const FS_CURRENT_AS_PATHNAME: i64 = 32; const FS_CURRENT_MODE_MASK: i64 = 240; const FS_SKIP_DOTS: i64 = 4096; +/// Widens locals whose indexed-array element type joins to `mixed` across the loop body's +/// push sites (issue #452). Loop bodies are checked in a single pass, so without this the +/// entry environment types an early push site against the pre-promotion element type even +/// though the back edge brings the promoted array around; fixing the element type to +/// `mixed` up front makes every push site see the fixed-point type. +fn widen_loop_grown_array_pushes(body: &[Stmt], update: Option<&Stmt>, env: &mut TypeEnv) { + let names = + crate::types::checker::loop_grown_mixed_array_pushes(body, update, &|name| { + env.get(name).cloned() + }); + for name in names { + env.insert(name, PhpType::Array(Box::new(PhpType::Mixed))); + } +} + /// Restores a narrowed variable in the environment to its previously saved type after a guarded /// branch, removing it when it had no prior type. Used to keep `if`/`else` type narrowing scoped /// to its branch. @@ -131,6 +146,9 @@ impl Checker { "by-reference foreach over Iterator/IteratorAggregate objects or iterable-typed values is not supported; use an array source or remove &", )); } + // Widen after the key/value bindings are in the environment so a push of + // the foreach value variable joins with its real element type. + widen_loop_grown_array_pushes(body, None, env); let errors = self.check_break_continue_target_body(body, env); if errors.is_empty() { Ok(()) @@ -257,6 +275,7 @@ impl Checker { } } StmtKind::DoWhile { body, condition } => { + widen_loop_grown_array_pushes(body, None, env); let errors = self.check_break_continue_target_body(body, env); self.infer_type_with_assignment_effects(condition, env)?; if errors.is_empty() { @@ -266,6 +285,7 @@ impl Checker { } } StmtKind::While { condition, body } => { + widen_loop_grown_array_pushes(body, None, env); self.infer_type_with_assignment_effects(condition, env)?; let errors = self.check_break_continue_target_body(body, env); if errors.is_empty() { @@ -283,6 +303,7 @@ impl Checker { if let Some(s) = init { self.check_stmt(s, env)?; } + widen_loop_grown_array_pushes(body, update.as_deref(), env); if let Some(c) = condition { self.infer_type_with_assignment_effects(c, env)?; } diff --git a/tests/codegen/array_basics.rs b/tests/codegen/array_basics.rs index a8dbb78d67..d5b3532ce2 100644 --- a/tests/codegen/array_basics.rs +++ b/tests/codegen/array_basics.rs @@ -240,6 +240,117 @@ echo $a[0]->n . "|" . $a[5]->n . "|" . count($a); assert_eq!(out, "p0|p5|6"); } +/// Regression for #452: pushing heterogeneous scalars (int then float) into an empty array +/// inside a loop promotes the array to mixed-element storage on iteration 1, but the earlier +/// push site was lowered against the stale pre-promotion type and wrote an unboxed scalar +/// into the mixed array on iteration 2. Reading that element then dereferenced the raw +/// scalar as a boxed cell pointer and crashed. The loop body must see the fixed-point +/// (back-edge) element type so every push site boxes correctly. +#[test] +fn test_loop_grown_mixed_array_element_read() { + let out = compile_and_run( + r#"` through such a +/// variable; a spurious widen made its typed-property storeback fail to compile. +#[test] +fn test_loop_rebuild_of_typed_array_not_spuriously_widened() { + let out = compile_and_run( + r#"attachIterator($it); +$multi->detachIterator($it); +echo $multi->countIterators(); +"#, + ); + assert_eq!(out, "0"); +} + +/// Regression companion for #452: heterogeneous scalars carried through loop-defined +/// variables (literal assignments inside the body) must still widen the pushed array — +/// the literal-assignment scan supplies the evidence the loop-entry lookup lacks. +#[test] +fn test_loop_grown_mixed_array_via_local_literals() { + let out = compile_and_run( + r#"