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..3e9257a9 --- /dev/null +++ b/tests/custom/99_bugs/59_format_gc_live_values @@ -0,0 +1,297 @@ +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. +Use sprintf() so the test does not depend on partial output after exit(). + +-- Testcase -- +{% + sprintf('%J', [ + proto({}, { tostring: () => exit(3) }), + proto({}, { tostring: () => { print('converter called\n'); return 'B'; } }), + { n: 7 } + ]); + print('not reached\n'); +%} +-- End -- + +-- 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 +-- 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..8110f76b 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,24 @@ 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; + + /* exit() may have released uv while resetting the VM stack. */ + if (vm->exception.type == EXCEPTION_EXIT) + goto out; + + constified = ucv_set_constant(uv, true); + } ucv_set_mark(uv); @@ -2164,6 +2200,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 +3134,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); }