diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b3f491d..4ac48904 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,36 @@ version number before tagging the release. ## [current] +### Fixed + +- **A heap-string struct field assigned from a borrowed value was freed at + teardown, a double free / free of rodata.** When a heap-string field is + assigned from a bare heap-tracked local (`a.value = v`), the field's + `_heap_` tracker was hard-coded to 1. But a local classified as a + heap-string var can hold a *borrowed* value at runtime, e.g. one returned by + a function that passes a parameter or a literal straight through, leaving its + `_heap_` at 0. The struct destructor then freed a pointer the program + never owned: a literal (free of read-only data) or a value still owned + elsewhere. It surfaced downstream as a callback/FFI crash: a hook returns a + literal `string`, the engine threads it through `-> string` helpers into a + heap-boxed struct field, and teardown aborted with `invalid pointer` / + SIGSEGV. The field store now *moves* the source var's runtime ownership + (`_heap_ = _heap_; _heap_ = 0`) instead of asserting 1, so a + borrowed value is not freed and a genuinely-owned one is still freed exactly + once. Regression follow-up to #1866/#1879. + + Reading the tracker only works if the tracker is maintained, and for one + shape it was not. When a heap-string var's value escapes into a container or + a struct field, its own frees are suppressed (the recipient may have kept the + pointer), and on that path the assignment left `_heap_` at its stale 0 + on the reasoning that nothing would read it. The field store now reads it, so + a genuinely owned value (`s = strbuilder.finish(b)` then `n.text = s`) moved + a 0 into the field tracker and the destructor never reclaimed the buffer. + `std.message` leaked 60 allocations exactly this way, caught by the macOS + leaks gate. The flag is the ownership token, so it is now recorded whether or + not this scope is the one that acts on it; the escape-suppressed frees are + unchanged. + ## [0.648.0] ### Fixed diff --git a/compiler/codegen/codegen_stmt.c b/compiler/codegen/codegen_stmt.c index 66447497..61e5af00 100644 --- a/compiler/codegen/codegen_stmt.c +++ b/compiler/codegen/codegen_stmt.c @@ -1491,6 +1491,9 @@ static int is_ptr_struct_param(CodeGenerator* gen, const char* name) { * is safe regardless and is what stops the leak; a repeated assignment * through a nested path can still drop the earlier value, which is strictly * better than a segfault and matches what a pointer parameter already does. */ +static int emit_field_tracker_from_rhs(CodeGenerator* gen, ASTNode* rhs, + const char* tracker_lvalue); + static int emit_nested_field_heap_assign(CodeGenerator* gen, ASTNode* lhs, ASTNode* rhs, ASTNode* obj) { Type* obj_type = obj->node_type; @@ -1525,14 +1528,44 @@ static int emit_nested_field_heap_assign(CodeGenerator* gen, ASTNode* lhs, char tgt[32]; snprintf(tgt, sizeof(tgt), "_ae_ntgt%d", nested_tgt_seq++); + char tracker_lv[256]; + snprintf(tracker_lv, sizeof(tracker_lv), "%s->_heap_%s", tgt, lhs->value); print_indent(gen); fprintf(gen->output, "{ %s* %s = ", obj_type->element_type->struct_name, tgt); generate_expression(gen, obj); fprintf(gen->output, "; %s->%s = ", tgt, lhs->value); generate_expression(gen, rhs); - fprintf(gen->output, "; %s->_heap_%s = %d; }\n", - tgt, lhs->value, rhs_is_heap ? 1 : 0); + fprintf(gen->output, ";"); + /* Move the source var's runtime ownership when the RHS is a heap-var + * identifier (it may hold a borrow); otherwise the static classification. */ + if (!emit_field_tracker_from_rhs(gen, rhs, tracker_lv)) { + fprintf(gen->output, " %s = %d;", tracker_lv, rhs_is_heap ? 1 : 0); + } + fprintf(gen->output, " }\n"); + return 1; +} + +/* When a heap-string struct field is assigned from a bare heap-tracked-var + * identifier, the field's `_heap_` tracker must take that variable's + * RUNTIME ownership, not a static 1. A local classified as a heap-string var + * can still hold a BORROWED value at runtime (e.g. it was assigned from a + * function that returns a borrowed/literal pass-through, leaving `_heap_ + * == 0`). Hard-coding the field tracker to 1 then makes the struct destructor + * free a pointer the program never owned — a literal (free of rodata) or a + * value still owned elsewhere (double free). This is the field-store analogue + * of the `_heap_dest = _heap_src` ownership move used for `dest = src` aliases. + * + * Writes ` = _heap_;` then disowns the source + * (`_heap_ = 0;`) so the freeing duty transfers to the field and the + * source is not also freed at scope exit. Returns 1 if it handled the RHS + * (a bare heap-var identifier); 0 to fall back to the static rhs_is_heap. */ +static int emit_field_tracker_from_rhs(CodeGenerator* gen, ASTNode* rhs, + const char* tracker_lvalue) { + if (!gen || !rhs || rhs->type != AST_IDENTIFIER || !rhs->value) return 0; + if (!is_heap_string_var(gen, rhs->value)) return 0; + fprintf(gen->output, " %s = _heap_%s; _heap_%s = 0;", + tracker_lvalue, rhs->value, rhs->value); return 1; } @@ -1610,6 +1643,9 @@ static int emit_struct_field_heap_assign(CodeGenerator* gen, ASTNode* lhs, ASTNo matching_field->node_type->kind != TYPE_STRING) return 0; int rhs_is_heap = is_heap_string_expr(gen, rhs); + char tracker_lv[256]; + snprintf(tracker_lv, sizeof(tracker_lv), "%s%s_heap_%s", + obj->value, acc, lhs->value); print_indent(gen); if (!tracker_is_trustworthy) { /* #1873: store and SET the tracker (so the destructor still reclaims @@ -1621,8 +1657,13 @@ static int emit_struct_field_heap_assign(CodeGenerator* gen, ASTNode* lhs, ASTNo * the caller can use heap.new to get the releasing behaviour. */ fprintf(gen->output, "{ %s%s%s = ", obj->value, acc, lhs->value); generate_expression(gen, rhs); - fprintf(gen->output, "; %s%s_heap_%s = %d; }\n", - obj->value, acc, lhs->value, rhs_is_heap ? 1 : 0); + fprintf(gen->output, ";"); + /* Move the source var's runtime ownership when the RHS is a heap-var + * identifier (it may hold a borrow); otherwise the static class. */ + if (!emit_field_tracker_from_rhs(gen, rhs, tracker_lv)) { + fprintf(gen->output, " %s = %d;", tracker_lv, rhs_is_heap ? 1 : 0); + } + fprintf(gen->output, " }\n"); return 1; } fprintf(gen->output, @@ -1630,12 +1671,11 @@ static int emit_struct_field_heap_assign(CodeGenerator* gen, ASTNode* lhs, ASTNo obj->value, acc, lhs->value, obj->value, acc, lhs->value); generate_expression(gen, rhs); - fprintf(gen->output, - "; if (%s%s_heap_%s) aether_heap_str_free(_tmp_old); " - "%s%s_heap_%s = %d; }\n", - obj->value, acc, lhs->value, - obj->value, acc, lhs->value, - rhs_is_heap ? 1 : 0); + fprintf(gen->output, "; if (%s) aether_heap_str_free(_tmp_old);", tracker_lv); + if (!emit_field_tracker_from_rhs(gen, rhs, tracker_lv)) { + fprintf(gen->output, " %s = %d;", tracker_lv, rhs_is_heap ? 1 : 0); + } + fprintf(gen->output, " }\n"); return 1; } @@ -4987,9 +5027,25 @@ void generate_statement(CodeGenerator* gen, ASTNode* stmt) { emit_unwind_track_local(gen, stmt->value); fprintf(gen->output, " }\n"); } else { - fprintf(gen->output, "%s = ", stmt->value); + /* Record the value's heapness even though this + * var's own frees are escape-suppressed. The + * tracker used to be left at its stale value here + * on the reasoning that nothing would read it: the + * defer-free and the reassignment wrapper are both + * suppressed for an escaped var, so the flag was + * dead weight. It is not dead any more. A struct + * field store now MOVES this flag into the field's + * own tracker to decide whether the destructor + * frees, so a stale 0 on a genuinely owned value + * (`s = strbuilder.finish(b)` then `n.text = s`) + * tells the destructor to leave it alone and the + * buffer leaks. The flag is the ownership token; + * it has to be true whether or not this scope is + * the one that acts on it. */ + fprintf(gen->output, "{ %s = ", stmt->value); generate_expression(gen, stmt->children[0]); - fprintf(gen->output, ";\n"); + fprintf(gen->output, "; _heap_%s = %d; }\n", + stmt->value, rhs_is_heap ? 1 : 0); } } else if (var_is_string && stmt->child_count > 0) { // Defensive: if the hoist somehow missed this diff --git a/tests/integration/heap_field_borrowed_var_no_double_free/prog.ae b/tests/integration/heap_field_borrowed_var_no_double_free/prog.ae new file mode 100644 index 00000000..92e3c5e6 --- /dev/null +++ b/tests/integration/heap_field_borrowed_var_no_double_free/prog.ae @@ -0,0 +1,83 @@ +// A heap-string struct field assigned from a heap-tracked LOCAL that at +// runtime holds a BORROWED value must not be freed by the struct destructor. +// +// `passthru` returns its parameter straight through, so it is classified +// non-heap: `borrowed` is a heap-string var (it is reassignable to owned +// strings elsewhere in a real program) but its runtime `_heap_borrowed` is 0. +// Before the fix, `a.value = borrowed` hard-coded `_heap_value = 1`, so +// `heap.free(a)` freed a value the program never owned — a literal string +// (free of rodata) — aborting with SIGSEGV / "invalid pointer" at teardown. +// The store must instead MOVE the source's runtime ownership: +// a->value = borrowed; a->_heap_value = _heap_borrowed; _heap_borrowed = 0; +// +// This is the FFI/closure-callback shape a downstream sanitizer hit: a hook +// returns a literal, the engine threads it through `-> string` helpers into a +// heap-boxed attr's string field, and teardown double-freed it. +// +// The `owned` half proves the fix does not regress the leak it protects: +// a genuinely-heap value is still freed exactly once. +// +// `owned` alone is not enough, and the reason is worth stating: the compiler +// constant-folds `string.concat("https://", "cdn.example/style.css")` into a +// literal, so that half runs with `_heap_owned == 0` and exercises the +// borrowed path a second time rather than the owned one. `runtime_owned` is +// built through a call the folder cannot see through, so it is genuinely heap +// at runtime -- and it is the shape that leaked: for a var whose value escapes +// into a struct field, the assignment used to leave `_heap_` at its stale +// 0, and moving that 0 into the field tracker told the destructor the buffer +// was borrowed. std.message leaked 60 allocations exactly this way. + +import std.io +import std.string +import std.strbuilder + +struct Attr { + name: string + value: string +} + +fn passthru(u: string) -> string { + return u +} + +// Built through a call, so the constant folder cannot reduce it to a literal: +// the result is genuinely heap-allocated at runtime. +fn build_owned(n: int) -> string { + b = strbuilder.new(16) + strbuilder.append(b, "https://cdn.example/") + strbuilder.append_int(b, n) + return strbuilder.finish(b) +} + +main() { + // Borrowed: value the program does not own. Must NOT be freed at teardown. + a = heap.new(Attr) + a.name = "src" + borrowed = passthru("https://cdn.example/logo.png") + ap = a as *Attr + ap.value = borrowed + println("borrowed: ${a.value}") + heap.free(a) + + // Owned: genuinely-heap value. Must be freed exactly once (no leak). + b = heap.new(Attr) + b.name = "href" + owned = string.concat("https://", "cdn.example/style.css") + bp = b as *Attr + bp.value = owned + println("owned: ${b.value}") + heap.free(b) + + // Genuinely heap at runtime, and escaping into a struct field. The field + // tracker must receive 1 so the destructor reclaims it exactly once: + // freeing nothing here is the leak, freeing twice is the crash above. + c = heap.new(Attr) + c.name = "src" + runtime_owned = build_owned(7) + cp = c as *Attr + cp.value = runtime_owned + println("runtime_owned: ${c.value}") + heap.free(c) + + println("done") +} diff --git a/tests/integration/heap_field_borrowed_var_no_double_free/test_heap_field_borrowed_var_no_double_free.sh b/tests/integration/heap_field_borrowed_var_no_double_free/test_heap_field_borrowed_var_no_double_free.sh new file mode 100755 index 00000000..741a7934 --- /dev/null +++ b/tests/integration/heap_field_borrowed_var_no_double_free/test_heap_field_borrowed_var_no_double_free.sh @@ -0,0 +1,94 @@ +#!/bin/sh +# A heap-string struct field assigned from a heap-tracked local that holds a +# BORROWED value at runtime must move the source's runtime ownership into the +# field tracker, not hard-code it to 1. Hard-coding 1 made the destructor free +# a value the program never owned (a literal -> free of rodata), aborting at +# teardown. This is the closure/FFI-callback shape a downstream HTML sanitizer +# hit: a hook returns a literal, threaded through `-> string` helpers into a +# heap-boxed attr field, double-freed on free. +# +# Running to completion is half the assertion (a double free / rodata free +# aborts with SIGSEGV or SIGABRT). The emitted ownership move is checked +# directly too, so this means something on CI platforms with no leak checker. + +set -e + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)" +AE="$ROOT/build/ae" +AETHERC="$ROOT/build/aetherc" +[ -x "$AE" ] || { echo " [SKIP] heap_field_borrowed_var_no_double_free: build/ae missing"; exit 0; } + +TMP="$(mktemp -d)" +cleanup() { rm -rf "$TMP"; } +trap cleanup EXIT + +# 1. Generated C moves the source var's runtime tracker into the field, rather +# than writing a literal 1. The borrowed store must NOT read `= 1`. +if ! "$AETHERC" "$SCRIPT_DIR/prog.ae" "$TMP/out.c" > "$TMP/gen.log" 2>&1; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: codegen failed" + sed 's/^/ /' "$TMP/gen.log" | head -8 + exit 1 +fi +if ! grep -q "_heap_value = _heap_borrowed" "$TMP/out.c"; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: borrowed store did not move the runtime tracker" + echo " expected '..._heap_value = _heap_borrowed; _heap_borrowed = 0;'" + grep -n "value = borrowed\|_heap_value" "$TMP/out.c" | sed 's/^/ /' | head -6 + exit 1 +fi +if grep -q "value = borrowed; ap->_heap_value = 1" "$TMP/out.c"; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: borrowed store still hard-codes _heap_value = 1" + exit 1 +fi + +# 1b. The mirror image: a var holding a GENUINELY owned value must reach the +# field tracker as 1, or the destructor frees nothing and the buffer leaks. +# For a var whose value escapes into a struct field the assignment used to +# leave `_heap_` at its stale 0, so the move carried a 0 and the leak +# was silent on every platform without a leak checker. Asserted on the +# generated C so it fails on those platforms too. +if ! grep -q "runtime_owned = build_owned(7); _heap_runtime_owned = 1" "$TMP/out.c"; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: an owned value that escapes" + echo " into a field left its tracker stale, so the field tracker takes 0" + echo " and the destructor never frees it" + grep -n "runtime_owned" "$TMP/out.c" | sed 's/^/ /' | head -6 + exit 1 +fi +if ! grep -q "_heap_value = _heap_runtime_owned" "$TMP/out.c"; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: owned store did not move the runtime tracker" + grep -n "value = runtime_owned\|_heap_value" "$TMP/out.c" | sed 's/^/ /' | head -6 + exit 1 +fi + +# 2. It runs to completion — the actual bug was a teardown abort. Build a real +# binary and run it; a double free / rodata free crashes here. +if ! "$AE" build "$SCRIPT_DIR/prog.ae" -o "$TMP/prog" > "$TMP/build.log" 2>&1; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: build failed" + sed 's/^/ /' "$TMP/build.log" | head -8 + exit 1 +fi +if ! "$TMP/prog" > "$TMP/run.out" 2>&1; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: program aborted at runtime (rc $?)" + sed 's/^/ /' "$TMP/run.out" | head -8 + exit 1 +fi +if ! grep -q "^done$" "$TMP/run.out"; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: did not reach 'done'" + sed 's/^/ /' "$TMP/run.out" | head -8 + exit 1 +fi + +# 3. Freed exactly once, where a leak checker is available. leaks(1) is the one +# that works on macOS; Linux CI covers the same program under Valgrind. +if [ "$(uname -s)" = Darwin ] && command -v leaks >/dev/null 2>&1; then + if ! MallocStackLogging=1 leaks --atExit -- "$TMP/prog" > "$TMP/leaks.out" 2>&1; then + if grep -qE "[1-9][0-9]* leak(s)? for" "$TMP/leaks.out"; then + echo " [FAIL] heap_field_borrowed_var_no_double_free: the owned value leaked" + grep -E "leaks? for" "$TMP/leaks.out" | sed 's/^/ /' | head -3 + exit 1 + fi + fi +fi + +echo " [PASS] heap_field_borrowed_var_no_double_free: borrowed field-store moves runtime ownership; no teardown double-free" +exit 0