From 9eb8baef9fd1d6a678303d1d6ee8016d9b0d2912 Mon Sep 17 00:00:00 2001 From: "shoney.arickathil" Date: Wed, 19 Aug 2026 19:36:49 +0200 Subject: [PATCH] fix(compiler): storing an owned value in a container is a MOVE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The disclosed "move-on-push" gap detonated on iteration 16's route-table pattern: `push(self.routes, r)` moved the Route into the container while the `take r` parameter's scope-end DROP still fired — the container's own drop plan (multi_free) then freed the element a second time. ASan: SEGV in class_free during trap unwind; latent until now because pushed elements were Texts, which copy at the boundary (2026-08-14). owner.ml analyze_call: `push`'s value slot and `set`'s key/value slots now TRANSFER an Owned, non-copy-stored place (record_move, exactly the take-arg shape), so the pusher's drop disappears. Text/json.Value keep the copy-store path (stores_by_copy) and the caller still drops the fresh copy. Traced (gc) values remain exempt (tracing owns them). A user-declared push/set fn of the same name wins, per the builtin shadowing rule. Pinned by tests/corpus/run/container-owned-move (route table: interface- typed field values pushed via take params, dispatched by ICALL, mixed with Text pushes) — the exact iteration-16 shape, ASan-clean. Verified: woc-test 540/0; oop-e2e 88/0; log-watcher 7/0; employee 8/0. Co-Authored-By: Claude Opus 5 (1M context) --- compiler/src/owner.ml | 27 ++++++++++- .../run/container-owned-move/fixture.out | 4 ++ .../run/container-owned-move/fixture.wo | 45 +++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 tests/corpus/run/container-owned-move/fixture.out create mode 100644 tests/corpus/run/container-owned-move/fixture.wo diff --git a/compiler/src/owner.ml b/compiler/src/owner.ml index 6f1f778..6b83673 100644 --- a/compiler/src/owner.ml +++ b/compiler/src/owner.ml @@ -1304,8 +1304,27 @@ and analyze_call (ctx : ctx) (call_e : Ast.expr) (callee : Ast.expr) (args : Ast (* transfers last *) (* iteration 7b deleted the `push`-of-a-gc-value special case (an RC_INC escape site): a traced value stored into a container needs no - bookkeeping — tracing finds it through the container. The bug class the - old special case guarded against cannot recur without RC. *) + bookkeeping — tracing finds it through the container. + + Storing an OWNED, non-copied value into a container is a MOVE, exactly + like a `take` argument: the container owns the element and frees it in + its own drop plan (runtime/src/gc.c multi_free/map_free), so the caller + dropping it too was a double free. This was the disclosed + "move-on-push" gap — it stayed latent while pushed elements were Texts + (copied at the boundary, 2026-08-14) and detonated the moment iteration + 16's route-table pattern pushed a CLASS value (`push(self.routes, r)` + plus the take-param's scope-end DROP = the container's element freed + twice). `push`'s value slot and `set`'s key/value slots transfer; + Text/json.Value stay copy-stored (`stores_by_copy`), so their fresh- + value drop is still the caller's. A user-declared fn of the same name + wins, exactly like the builtin table's shadowing rule. *) + let container_store_slot (i : int) : bool = + match callee.kind with + | Ident "push" -> i = 1 && Types.StringMap.find_opt "push" ctx.syms.Types.free_fns = None + | Ident "set" -> + (i = 1 || i = 2) && Types.StringMap.find_opt "set" ctx.syms.Types.free_fns = None + | _ -> false + in List.iteri (fun i a -> match place_of a with @@ -1315,6 +1334,10 @@ and analyze_call (ctx : ctx) (call_e : Ast.expr) (callee : Ast.expr) (args : Ast if conv = Take then (if transfer ctx p ~what:(Printf.sprintf "cannot be passed to `take %s`" pname) then record_move ctx p (MvArg pname)) + else if container_store_slot i && place_class ctx p = Owned + && not (stores_by_copy ctx p) then + (if transfer ctx p ~what:"cannot be stored in a container" then + record_move ctx p (MvArg "element")) ) args; record_drop ctx ~node:call_e.id ~pos:call_e.pos ~kind:DLiveMask diff --git a/tests/corpus/run/container-owned-move/fixture.out b/tests/corpus/run/container-owned-move/fixture.out new file mode 100644 index 0000000..fb02566 --- /dev/null +++ b/tests/corpus/run/container-owned-move/fixture.out @@ -0,0 +1,4 @@ +101 +8 +-1 +2 diff --git a/tests/corpus/run/container-owned-move/fixture.wo b/tests/corpus/run/container-owned-move/fixture.wo new file mode 100644 index 0000000..4ce8252 --- /dev/null +++ b/tests/corpus/run/container-owned-move/fixture.wo @@ -0,0 +1,45 @@ +-- Storing an OWNED class value into a container is a MOVE (iteration 16's +-- route-table pattern found the disclosed "move-on-push" double-free: the +-- container's drop plan frees elements, so the pusher must not also drop +-- what it pushed). Texts stay copy-stored — the mixed push below pins both. +interface Handler { + fn handle(x: Int) -> Int +} + +class AddOne { + n: Int + fn handle(x: Int) -> Int { return x + self.n } +} + +class Route { + path: Text + h: Handler +} + +class App { + routes: multi Route + names: multi Text + + fn add(take r: Route, name: Text) { + push(self.routes, r); + push(self.names, name); + } + + fn run(path: Text, x: Int) -> Int { + for r in self.routes { + if r.path == path { return r.h.handle(x); } + } + return -1; + } +} + +fn main(args: multi Text) -> Int { + let app = App { routes: [], names: [] }; + app.add(Route { path: "/a", h: AddOne { n: 7 } }, "a"); + app.add(Route { path: "/b", h: AddOne { n: 100 } }, "b"); + print_int(app.run("/b", 1)); + print_int(app.run("/a", 1)); + print_int(app.run("/x", 1)); + print_int(count(app.names)); + return 0; +}