From be870dd61b959b7e2142c7d06fcafedce0fa7fbf Mon Sep 17 00:00:00 2001 From: Jo-Philipp Wich Date: Sun, 11 Oct 2026 23:34:18 +0200 Subject: [PATCH 1/2] types: give the collector a dedicated mark bit The stringification cycle detection, the collector's reachability walk and ucv_free()'s reentrancy guard all share the single mark bit in the value header. A tostring callback executes arbitrary script code and may trigger a collection while the formatter has marked the active traversal path: the mark phase stops at the already marked container, its not-yet-formatted children are swept and the sweep clears the mark, defeating the cycle detection for the rest of the traversal. Push a reference to the value being stringified onto the VM stack to keep it reachable across callbacks, popping it again on the way out unless exit() has already reset the stack, and mark the container constant for the duration of its traversal so that callbacks cannot add, remove or replace its entries, restoring the original constness afterwards. Separate the collector's reachability mark into its own header bit so that the mark phase always descends into containers under stringification and the sweep never touches their cycle detection marks. The new bit reuses the top bit of the former 26 bit refcount field, reducing the refcount to 25 bits; the field is declared so that this top bit is taken on both endiannesses, and only a refcount above 33 million would ever have set it, so no realistic value is affected, `type`, `mark` and `ext_flag` keep their positions, and modules reaching the refcount through ucv_get()/ucv_put() are unaffected. This removes the need for the collector to be aware of the formatting state at all: no per-container bookkeeping in the thread context and no mark clear/restore passes in ucv_gc_common() are required, while explicit and automatic collection keep working from inside converters. Add regressions for explicit and automatic collection, arrays and objects, reentrant formatting with cycles and shared values, rejected mutations of containers under stringification, converter exceptions, exit() in a converter, converters modifying their own value, formatting of constant values and collection of unreachable cycles in a callback. Fixes `ucode -p 'let a; a = [ proto({}, { tostring: () => { gc(); return "A"; } }), { b: 7 } ]; a'` corrupting the output. --- include/ucode/types.h | 30 +- tests/custom/99_bugs/59_format_gc_live_values | 285 ++++++++++++++++++ types.c | 60 +++- 3 files changed, 363 insertions(+), 12 deletions(-) create mode 100755 tests/custom/99_bugs/59_format_gc_live_values diff --git a/include/ucode/types.h b/include/ucode/types.h index b096ae0c..04e27d31 100644 --- a/include/ucode/types.h +++ b/include/ucode/types.h @@ -27,7 +27,22 @@ #include -/* Value types and generic value header */ +/* Value types and generic value header. + * + * `mark` is the cycle-detection mark used by stringification and by ubus + * blobmsg conversion (and as a reentrancy guard in ucv_free()); `gc_mark` + * is the collector's reachability mark. The two cannot share a bit: a + * tostring callback may run the collector while the formatter has marked + * the active traversal path, and the mark phase must then descend into + * those containers instead of stopping at their marks. + * + * `gc_mark` reuses the top bit of the former 26 bit refcount field, + * reducing the refcount to 25 bits; a refcount high enough to have set that + * bit would exceed 33 million, so the change is invisible in practice and + * `type`, `mark` and `ext_flag` keep their positions. The field is + * declared so that this top bit is chosen on both endiannesses: little + * endian packs bitfields low to high, so gc_mark is declared last, while + * big endian packs them high to low, so it is declared first. */ typedef enum uc_type { UC_NULL, @@ -46,12 +61,23 @@ typedef enum uc_type { UC_SOURCE } uc_type_t; +#if __BYTE_ORDER == __LITTLE_ENDIAN +typedef struct uc_value { + uint32_t type:4; + uint32_t mark:1; + uint32_t ext_flag:1; + uint32_t refcount:25; + uint32_t gc_mark:1; +} uc_value_t; +#else typedef struct uc_value { uint32_t type:4; uint32_t mark:1; uint32_t ext_flag:1; - uint32_t refcount:26; + uint32_t gc_mark:1; + uint32_t refcount:25; } uc_value_t; +#endif struct uc_vm; diff --git a/tests/custom/99_bugs/59_format_gc_live_values b/tests/custom/99_bugs/59_format_gc_live_values new file mode 100755 index 00000000..35a082cd --- /dev/null +++ b/tests/custom/99_bugs/59_format_gc_live_values @@ -0,0 +1,285 @@ +Values being stringified must stay alive until the formatter is done with +them. A tostring callback may run the garbage collector while an array or +object is being traversed; the container under stringification is immutable +for the duration of the traversal, so callbacks cannot unlink or modify it. + +-- Testcase -- +{% + let values = [ + proto({}, { tostring: () => { gc(); return 'A'; } }), + { b: 7 } + ]; + + print(values, '\n'); + print(values[1].b, '\n'); +%} +-- End -- + +-- Expect stdout -- +[ "A", { "b": 7 } ] +7 +-- End -- + + +The same protection applies to object values. + +-- Testcase -- +{% + let values = { + a: proto({}, { tostring: () => { gc(); return 'A'; } }), + b: { n: 7 } + }; + + print(values, '\n'); + print(values.b.n, '\n'); +%} +-- End -- + +-- Expect stdout -- +{ "a": "A", "b": { "n": 7 } } +7 +-- End -- + + +Collection must preserve active cycle detection across nested formatting. +Repeated references outside the active path are formatted independently. + +-- Testcase -- +{% + let shared = { n: 7 }; + let values; + values = [ + proto({}, { tostring: () => { gc(); return sprintf('%J', values); } }), + shared, + shared + ]; + push(values, values); + + print(values, '\n'); + gc(); + print(values, '\n'); + print(shared.n, '\n'); +%} +-- End -- + +-- Expect stdout -- +[ "null", { "n": 7 }, { "n": 7 }, null ] +[ "null", { "n": 7 }, { "n": 7 }, null ] +7 +-- End -- + + +Automatic collection from a converter must retain the remaining values too. + +-- Testcase -- +{% + let values = [ + proto({}, { tostring: () => { + for (let i = 0; i < 100; i++) { + let garbage = {}; + garbage.self = garbage; + } + return 'A'; + } }), + { b: 7 } + ]; + + gc('start', 1); + print(values, '\n'); + gc('stop'); + print(values[1].b, '\n'); +%} +-- End -- + +-- Expect stdout -- +[ "A", { "b": 7 } ] +7 +-- End -- + + +Containers under stringification are immutable. Converters attempting to +unlink or modify them raise an exception and leave them intact, mutability +is restored once formatting is done. + +-- Testcase -- +{% + let outer; + let inner = [ + proto({}, { tostring: () => { outer[0] = null; gc(); return 'A'; } }), + { b: 7 } + ]; + outer = [ inner ]; + + try { + sprintf('%J', outer); + } + catch (e) { + print(e.type, ': ', e.message, '\n'); + } + + assert(outer[0] === inner && length(inner) == 2); + + let object; + object = { + a: proto({}, { tostring: () => { delete object.b; return 'A'; } }), + b: { n: 7 } + }; + + try { + sprintf('%J', object); + } + catch (e) { + print(e.type, ': ', e.message, '\n'); + } + + assert(object.b.n == 7); + push(outer, 2); + object.c = 3; + print(length(outer), ' ', join(',', keys(object)), '\n'); +%} +-- End -- + +-- Expect stdout -- +Type error: array value is immutable +Type error: object value is immutable +2 a,b,c +-- End -- + + +Formatting state is released after a converter throws an exception. + +-- Testcase -- +{% + let values = [ + proto({}, { tostring: () => { gc(); die('conversion failed'); } }), + { b: 7 } + ]; + + try { + sprintf('%J', values); + } + catch (e) { + assert(e.message == 'conversion failed'); + print(e.message, '\n'); + } + + proto(values[0], {}); + gc(); + push(values, 8); + print(values, '\n'); + print(values[1].b, '\n'); +%} +-- End -- + +-- Expect stdout -- +conversion failed +[ { }, { "b": 7 }, 8 ] +7 +-- End -- + + +Calling exit() from a converter discards the entire VM stack. The formatter +must not pop its own slot afterwards and must not invoke further converters. + +-- Testcase -- +{% + print([ + proto({}, { tostring: () => exit(3) }), + proto({}, { tostring: () => { print('converter called\n'); return 'B'; } }), + { n: 7 } + ], '\n'); + print('not reached\n'); +%} +-- End -- + +-- Expect stdout -- +[ { }, { }, { "n": 7 } ] +-- End (no-eol) -- + +-- Expect exitcode -- +3 +-- End -- + + +A converter may modify its own value, only the containers being traversed +are immutable. + +-- Testcase -- +{% + let calls = 0; + let value = proto({}, { tostring: function() { + calls++; + this.cache ??= 'cached'; + return this.cache; + } }); + + print([ value, value ], '\n'); + print(calls, ' ', value.cache, '\n'); +%} +-- End -- + +-- Expect stdout -- +[ "cached", "cached" ] +2 cached +-- End -- + + +Values which are constant on their own, such as module namespaces, are +formatted normally, may be repeated and remain constant afterwards. + +-- Testcase -- +import * as mod from "./files/mod.uc"; + +print([ mod, mod ], "\n"); + +try { + mod.b = 2; +} +catch (e) { + print(e.message, "\n"); +} + +print(mod, "\n"); +-- End -- + +-- File mod.uc -- +export let a = 1; +-- End -- + +-- Args -- +-R +-- End -- + +-- Expect stdout -- +[ { "a": 1 }, { "a": 1 } ] +object value is immutable +{ "a": 1 } +-- End -- + + +Explicit collection still reclaims unreachable cycles inside a converter. + +-- Testcase -- +{% + let values = [ + proto({}, { tostring: () => { + let garbage = {}; + garbage.self = garbage; + garbage = null; + let before = gc('count'); + gc(); + assert(gc('count') < before); + return 'A'; + } }), + { b: 7 } + ]; + + print(values, '\n'); + print(values[1].b, '\n'); +%} +-- End -- + +-- Expect stdout -- +[ "A", { "b": 7 } ] +7 +-- End -- diff --git a/types.c b/types.c index ab122bb9..31dde6e5 100644 --- a/types.c +++ b/types.c @@ -144,6 +144,24 @@ ucv_put_value(uc_value_t *uv, bool retain) ucv_free(uv, retain); } +/* The collector keeps its reachability marks in a dedicated bit, separate + * from the mark used for stringification cycle detection: a tostring + * callback may trigger a collection while the formatter has marked the + * active traversal path, and the mark phase must then descend into those + * marked containers instead of stopping at their marks. */ +static inline bool +ucv_is_gc_marked(uc_value_t *uv) +{ + return (((uintptr_t)uv & 3) == 0 && uv != NULL && uv->gc_mark == true); +} + +static inline void +ucv_set_gc_mark(uc_value_t *uv) +{ + if (((uintptr_t)uv & 3) == 0 && uv != NULL) + uv->gc_mark = true; +} + static void ucv_gc_mark(uc_value_t *uv); @@ -161,7 +179,7 @@ ucv_gc_mark(uc_value_t *uv) struct lh_entry *entry; size_t i; - if (ucv_is_marked(uv)) + if (ucv_is_gc_marked(uv)) return; switch (ucv_type(uv)) { @@ -169,7 +187,7 @@ ucv_gc_mark(uc_value_t *uv) array = ucv_as_array(uv); if (array->ref.next) - ucv_set_mark(uv); + ucv_set_gc_mark(uv); ucv_gc_mark(array->proto); @@ -182,7 +200,7 @@ ucv_gc_mark(uc_value_t *uv) object = ucv_as_object(uv); if (object->ref.next) - ucv_set_mark(uv); + ucv_set_gc_mark(uv); ucv_gc_mark(object->proto); @@ -196,7 +214,7 @@ ucv_gc_mark(uc_value_t *uv) function = closure->function; if (closure->ref.next) - ucv_set_mark(uv); + ucv_set_gc_mark(uv); for (i = 0; i < function->nupvals; i++) ucv_gc_mark(&closure->upvals[i]->header); @@ -215,7 +233,7 @@ ucv_gc_mark(uc_value_t *uv) uc_resource_ext_t *res = (uc_resource_ext_t *)uv; if (res->ref.next) - ucv_set_mark(uv); + ucv_set_gc_mark(uv); values = ucv_resource_values(res); @@ -398,7 +416,7 @@ ucv_get(uc_value_t *uv) if (uv == NULL || (uintptr_t)uv & 3) return uv; - assert(uv->refcount < 0x03ffffff); + assert(uv->refcount < 0x01ffffff); uv->refcount++; @@ -1929,6 +1947,8 @@ void ucv_to_stringbuf_formatted(uc_vm_t *vm, uc_stringbuf_t *pb, uc_value_t *uv, size_t depth, char pad_char, size_t pad_size) { bool json = (pad_char != '\0'); + bool constified = false; + size_t stackoff = 0; uc_resource_type_t *restype; uc_cfunction_t *cfunction; uc_function_t *function; @@ -1947,8 +1967,20 @@ ucv_to_stringbuf_formatted(uc_vm_t *vm, uc_stringbuf_t *pb, uc_value_t *uv, size return; } - if (vm != NULL && ucv_call_tostring(vm, pb, uv, json)) - return; + /* with a VM, keep the value on the stack while its tostring callback (or + * those of its children) may unlink it or run the collector, and make the + * container constant so those callbacks cannot modify it during the + * traversal; no callbacks are invoked once exit() has been called */ + if (vm != NULL) { + stackoff = vm->stack.count; + uc_vm_stack_push(vm, ucv_get(uv)); + + if (vm->exception.type != EXCEPTION_EXIT && + ucv_call_tostring(vm, pb, uv, json)) + goto out; + + constified = ucv_set_constant(uv, true); + } ucv_set_mark(uv); @@ -2164,6 +2196,14 @@ ucv_to_stringbuf_formatted(uc_vm_t *vm, uc_stringbuf_t *pb, uc_value_t *uv, size } ucv_clear_mark(uv); + + if (constified) + ucv_set_constant(uv, false); + +out: + /* exit() within a callback resets the entire stack, our slot included */ + if (vm != NULL && vm->stack.count > stackoff) + ucv_put(uc_vm_stack_pop(vm)); } static char * @@ -3090,8 +3130,8 @@ ucv_gc_common(uc_vm_t *vm, bool final) for (ref = vm->values.next; ref != &vm->values; ref = ref->next) { val = (uc_value_t *)((uintptr_t)ref - offsetof(uc_array_t, ref)); - if (ucv_is_marked(val)) - ucv_clear_mark(val); + if (val->gc_mark) + val->gc_mark = false; else ucv_free(val, true); } From 00091f829801d1c2dff8efa001de71f47db55c3b Mon Sep 17 00:00:00 2001 From: Jo-Philipp Wich Date: Sat, 17 Oct 2026 01:08:09 +0200 Subject: [PATCH 2/2] types: avoid accessing values after converter exit An exit() in a tostring converter resets the VM stack, dropping the formatter's root. A directly formatted temporary value may be freed before ucv_call_tostring() returns false, so falling back to the default traversal accesses freed memory. Check for EXCEPTION_EXIT after converter dispatch and leave through the existing cleanup path before constifying or traversing the value. Add a regression for a directly formatted temporary object. Use sprintf() in the nested exit regression so it checks the exit status and skipped converters without depending on partially emitted output. Suggested-by: darekhta Signed-off-by: Jo-Philipp Wich --- tests/custom/99_bugs/59_format_gc_live_values | 22 ++++++++++++++----- types.c | 4 ++++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/tests/custom/99_bugs/59_format_gc_live_values b/tests/custom/99_bugs/59_format_gc_live_values index 35a082cd..3e9257a9 100755 --- a/tests/custom/99_bugs/59_format_gc_live_values +++ b/tests/custom/99_bugs/59_format_gc_live_values @@ -180,21 +180,33 @@ conversion failed Calling exit() from a converter discards the entire VM stack. The formatter must not pop its own slot afterwards and must not invoke further converters. +Use sprintf() so the test does not depend on partial output after exit(). -- Testcase -- {% - print([ + sprintf('%J', [ proto({}, { tostring: () => exit(3) }), proto({}, { tostring: () => { print('converter called\n'); return 'B'; } }), { n: 7 } - ], '\n'); + ]); print('not reached\n'); %} -- End -- --- Expect stdout -- -[ { }, { }, { "n": 7 } ] --- End (no-eol) -- +-- Expect exitcode -- +3 +-- End -- + + +An exiting converter may release a directly formatted temporary value. +The formatter must not access it again after the converter returns. + +-- Testcase -- +{% + print(proto({}, { tostring: () => exit(3) })); + print('not reached\n'); +%} +-- End -- -- Expect exitcode -- 3 diff --git a/types.c b/types.c index 31dde6e5..8110f76b 100644 --- a/types.c +++ b/types.c @@ -1979,6 +1979,10 @@ ucv_to_stringbuf_formatted(uc_vm_t *vm, uc_stringbuf_t *pb, uc_value_t *uv, size ucv_call_tostring(vm, pb, uv, json)) goto out; + /* exit() may have released uv while resetting the VM stack. */ + if (vm->exception.type == EXCEPTION_EXIT) + goto out; + constified = ucv_set_constant(uv, true); }