From 4711d3c2c5dcf8f2787e46f9e46a3ac3f6f3f9cc Mon Sep 17 00:00:00 2001 From: darekhta Date: Thu, 8 Oct 2026 21:55:18 +0300 Subject: [PATCH] lib: reject mutations of the container being sorted A sort() comparator, or a __tostring__ metamethod invoked by the default comparator, runs script code while qsort() operates on the array entry storage. Growing the array from there reallocates the entries behind qsort()'s back and shrinking it leaves qsort() working on stale slots, both leading to use-after-free and crashes. Sorting objects is affected the same way since the hash table entry pointers are kept across the comparator calls, which adding or deleting keys invalidates. Mark the container immutable for the duration of the sort by reusing the constant flag: all script-level mutation paths already raise a type error for constant values, so push(), pop(), shift(), unshift(), splice(), index assignments, delete and nested sort() calls on the container being sorted now fail with "array value is immutable" instead of corrupting memory. The flag is cleared again once qsort() returns, also when the comparator raised an exception. Extend the constant check in types.c to the array mutators which did not honour it yet, so that the lock is complete at the C level as well. As a consequence, ucv_array_set(), ucv_array_pop(), ucv_array_shift(), ucv_array_unshift() and ucv_array_delete() now refuse constant arrays like ucv_array_push() and ucv_object_add() already did, and the ucv_array_sort*() and ucv_object_sort*() functions become no-ops for constant containers. Also check for a pending exception after each string conversion in the default comparator, since a failing __tostring__ metamethod previously led to comparing a NULL string, and skip the default comparator as well once an exception is pending. Fixes `ucode -p 'a=[3,2,1]; sort(a, (x, y) => { for (let i = 0; i < 1000; i++) push(a, i); return x - y; })'` crashing. Signed-off-by: darekhta --- lib.c | 36 +++-- tests/custom/03_stdlib/16_sort | 280 +++++++++++++++++++++++++++++++++ types.c | 25 ++- 3 files changed, 321 insertions(+), 20 deletions(-) diff --git a/lib.c b/lib.c index f0f2c4d1..d35c4898 100644 --- a/lib.c +++ b/lib.c @@ -1706,11 +1706,11 @@ typedef struct { } sort_ctx_t; static int -default_cmp(uc_value_t *v1, uc_value_t *v2, uc_vm_t *vm) +default_cmp(uc_value_t *v1, uc_value_t *v2, sort_ctx_t *ctx) { - char *s1, *s2; - bool f1, f2; - int res; + char *s1 = NULL, *s2 = NULL; + bool f1 = false, f2 = false; + int res = 0; /* when both operands are numeric then compare numerically */ if ((ucv_type(v1) == UC_INTEGER || ucv_type(v1) == UC_DOUBLE) && @@ -1721,11 +1721,21 @@ default_cmp(uc_value_t *v1, uc_value_t *v2, uc_vm_t *vm) } /* otherwise convert both operands to strings and compare lexically */ - s1 = uc_cast_string(vm, &v1, &f1); - s2 = uc_cast_string(vm, &v2, &f2); + s1 = uc_cast_string(ctx->vm, &v1, &f1); + + if (ctx->vm->exception.type != EXCEPTION_NONE) + goto out; + + s2 = uc_cast_string(ctx->vm, &v2, &f2); + + if (ctx->vm->exception.type != EXCEPTION_NONE) + goto out; res = strcmp(s1, s2); +out: + ctx->ex = (ctx->vm->exception.type != EXCEPTION_NONE); + if (f1) free(s1); if (f2) free(s2); @@ -1739,12 +1749,12 @@ array_sort_fn(uc_value_t *v1, uc_value_t *v2, void *ud) sort_ctx_t *ctx = ud; int res; - if (!ctx->fn) - return default_cmp(v1, v2, ctx->vm); - if (ctx->ex) return 0; + if (!ctx->fn) + return default_cmp(v1, v2, ctx); + uc_vm_ctx_push(ctx->vm); uc_vm_stack_push(ctx->vm, ucv_get(ctx->fn)); uc_vm_stack_push(ctx->vm, ucv_get(v1)); @@ -1774,12 +1784,12 @@ object_sort_fn(const char *k1, uc_value_t *v1, const char *k2, uc_value_t *v2, sort_ctx_t *ctx = ud; int res; - if (!ctx->fn) - return strcmp(k1, k2); - if (ctx->ex) return 0; + if (!ctx->fn) + return strcmp(k1, k2); + uc_vm_ctx_push(ctx->vm); uc_vm_stack_push(ctx->vm, ucv_get(ctx->fn)); uc_vm_stack_push(ctx->vm, ucv_string_new(k1)); @@ -1808,6 +1818,8 @@ object_sort_fn(const char *k1, uc_value_t *v1, const char *k2, uc_value_t *v2, * If no sort function is provided, a default ascending sort order is applied. * * The input array is sorted in-place, no copy is made. + * The input is temporarily immutable while sorting. Attempts to modify it + * from the comparator or a string conversion method raise an exception. * * The custom sort function is repeatedly called until the entire array is * sorted. It will receive two values as arguments and should return a value diff --git a/tests/custom/03_stdlib/16_sort b/tests/custom/03_stdlib/16_sort index 685cb182..b0054569 100644 --- a/tests/custom/03_stdlib/16_sort +++ b/tests/custom/03_stdlib/16_sort @@ -118,3 +118,283 @@ In [anonymous function](), line 2, byte 37: -- End -- + + +Growing an array from its comparator must not invalidate the storage +being sorted. The array becomes mutable again after the exception. + +-- Testcase -- +{% + let expected = []; + + for (let i = 0; i < 64; i++) + push(expected, i); + + let input = reverse(expected); + let calls = 0; + + try { + sort(input, (a, b) => { + calls++; + + for (let i = 0; i < 1000; i++) + push(input, i); + + return a - b; + }); + die("Mutation was not rejected"); + } + catch (e) { + assert(e.type == "Type error" && e.message == "array value is immutable"); + } + + assert(calls == 1 && length(input) == 64); + assert(join(",", sort(input)) == join(",", expected)); + push(input, 64); + assert(pop(input) == 64 && length(input) == 64); + print("Array growth rejected; mutability restored\n"); +%} +-- End -- + +-- Expect stdout -- +Array growth rejected; mutability restored +-- End -- + + +Other array mutations, including recursive sorting, are rejected too. + +-- Testcase -- +{% + for (let mutate in [ + (a) => pop(a), + (a) => shift(a), + (a) => unshift(a, 0), + (a) => splice(a, 0, 1), + (a) => splice(a, 0, 0, 4, 5), + (a) => { a[1000] = 4; }, + (a) => { a[0] = 4; }, + (a) => { delete a[0]; }, + (a) => sort(a) + ]) { + let input = [ 3, 2, 1 ]; + let calls = 0; + + try { + sort(input, (a, b) => { + calls++; + mutate(input); + return a - b; + }); + die("Mutation was not rejected"); + } + catch (e) { + assert(e.type == "Type error" && e.message == "array value is immutable"); + } + + assert(calls == 1 && join(",", sort(input)) == "1,2,3"); + push(input, 4); + assert(pop(input) == 4); + } + + print("Array mutations and recursive sorting rejected\n"); +%} +-- End -- + +-- Expect stdout -- +Array mutations and recursive sorting rejected +-- End -- + + +Growing or deleting object entries from a comparator must not invalidate +the hash table entry pointers saved by the sort operation. + +-- Testcase -- +{% + for (let mutate in [ + (o) => { for (let i = 0; i < 100; i++) o[`key${i}`] = i; }, + (o) => { delete o.a; }, + (o) => { o.a = 4; }, + (o) => sort(o) + ]) { + let input = { c: 3, b: 2, a: 1 }; + let calls = 0; + + try { + sort(input, (k1, k2) => { + calls++; + mutate(input); + return k1 < k2 ? -1 : k1 > k2 ? 1 : 0; + }); + die("Mutation was not rejected"); + } + catch (e) { + assert(e.type == "Type error" && e.message == "object value is immutable"); + } + + assert(calls == 1 && length(input) == 3); + assert(input.a == 1 && input.b == 2 && input.c == 3); + assert(join(",", keys(sort(input))) == "a,b,c"); + input.d = 4; + assert(delete input.d); + assert(length(input) == 3); + } + + print("Object mutations and recursive sorting rejected\n"); +%} +-- End -- + +-- Expect stdout -- +Object mutations and recursive sorting rejected +-- End -- + + +A comparator may handle a rejected mutation and continue sorting. It may +also sort and modify other collections, and modify nested values. + +-- Testcase -- +{% + let input = [ { n: 3 }, { n: 2 }, { n: 1 } ]; + let other = [ 2, 1 ]; + let object = { b: 2, a: 1 }; + let calls = 0; + + assert(sort(input, (a, b) => { + calls++; + a.seen = true; + b.seen = true; + push(other, 0); + sort(other); + object.c = 3; + sort(object); + + try { + push(input, null); + die("Mutation was not rejected"); + } + catch (e) { + assert(e.type == "Type error" && e.message == "array value is immutable"); + } + + return a.n - b.n; + }) == input); + + assert(calls > 0 && join(",", map(input, (v) => v.n)) == "1,2,3"); + assert(input[0].seen && input[1].seen && input[2].seen); + push(input, { n: 4 }); + assert(pop(input).n == 4); + assert(join(",", keys(object)) == "a,b,c"); + print("Handled mutation errors and independent sorts work\n"); +%} +-- End -- + +-- Expect stdout -- +Handled mutation errors and independent sorts work +-- End -- + + +The default comparator must reject mutations from tostring() methods and +propagate their exceptions before invoking any further conversions. + +-- Testcase -- +{% + let input; + let calls = 0; + let item = proto({}, { tostring: function() { + calls++; + push(input, 4); + return "item"; + } }); + input = [ item, item, item ]; + + try { + sort(input); + die("Mutation was not rejected"); + } + catch (e) { + assert(e.type == "Type error" && e.message == "array value is immutable"); + } + + assert(calls == 1 && length(input) == 3); + push(input, 4); + assert(pop(input) == 4); + print("String conversion mutation rejected; mutability restored\n"); +%} +-- End -- + +-- Expect stdout -- +String conversion mutation rejected; mutability restored +-- End -- + + +An exception in either operand's conversion stops all later conversions. + +-- Testcase -- +{% + for (let fail_at in [ 1, 2 ]) { + let calls = 0; + let item = proto({}, { tostring: function() { + if (++calls == fail_at) + die("Sort conversion failed"); + + return "item"; + } }); + let input = [ item, item, item ]; + + try { + sort(input); + die("Conversion exception was not propagated"); + } + catch (e) { + assert(e.message == "Sort conversion failed"); + } + + assert(calls == fail_at && length(input) == 3); + push(input, 4); + assert(pop(input) == 4); + } + + print("String conversion exceptions stop subsequent callbacks\n"); +%} +-- End -- + +-- Expect stdout -- +String conversion exceptions stop subsequent callbacks +-- End -- + + +Mutability is restored after successful sorting as well as comparator +exceptions, without changing the identity or prototype of the input. + +-- Testcase -- +{% + let prototype = { marker: true }; + let input = proto([ 3, 2, 1 ], prototype); + let object = proto({ c: 3, b: 2, a: 1 }, prototype); + + assert(sort(input) == input && proto(input) == prototype); + assert(sort(object) == object && proto(object) == prototype); + push(input, 4); + object.d = 4; + assert(join(",", input) == "1,2,3,4"); + assert(join(",", keys(object)) == "a,b,c,d"); + + for (let source in [ input, object ]) { + try { + sort(source, () => die("Sort comparator failed")); + die("Comparator exception was not propagated"); + } + catch (e) { + assert(e.message == "Sort comparator failed"); + } + } + + assert(pop(input) == 4); + assert(delete object.d); + assert(proto(input) == prototype && proto(object) == prototype); + print("Mutability, identity and prototypes preserved\n"); +%} +-- End -- + +-- Expect stdout -- +Mutability, identity and prototypes preserved +-- End -- diff --git a/types.c b/types.c index bee78893..ab122bb9 100644 --- a/types.c +++ b/types.c @@ -786,7 +786,7 @@ ucv_array_pop(uc_value_t *uv) uc_array_t *array = ucv_as_array(uv); uc_value_t *item; - if (ucv_type(uv) != UC_ARRAY || array->count == 0) + if (ucv_type(uv) != UC_ARRAY || ucv_is_constant(uv) || array->count == 0) return NULL; item = ucv_get(array->entries[array->count - 1]); @@ -815,7 +815,7 @@ ucv_array_shift(uc_value_t *uv) uc_array_t *array = ucv_as_array(uv); uc_value_t *item; - if (ucv_type(uv) != UC_ARRAY || array->count == 0) + if (ucv_type(uv) != UC_ARRAY || ucv_is_constant(uv) || array->count == 0) return NULL; item = ucv_get(array->entries[0]); @@ -831,7 +831,7 @@ ucv_array_unshift(uc_value_t *uv, uc_value_t *item) uc_array_t *array = ucv_as_array(uv); size_t i; - if (ucv_type(uv) != UC_ARRAY) + if (ucv_type(uv) != UC_ARRAY || ucv_is_constant(uv)) return NULL; uc_vector_extend(array, 1); @@ -860,10 +860,13 @@ ucv_array_sort_r(uc_value_t *uv, array_sort_ctx_t ctx = { .cmp = cmp, .ud = ud }; uc_array_t *array = ucv_as_array(uv); - if (ucv_type(uv) != UC_ARRAY || array->count <= 1) + if (ucv_type(uv) != UC_ARRAY || ucv_is_constant(uv) || array->count <= 1) return; + /* Comparators must not invalidate the storage used by qsort. */ + ucv_set_constant(uv, true); uc_vector_sort(array, ucv_array_sort_r_cb, &ctx); + ucv_set_constant(uv, false); } void @@ -871,10 +874,12 @@ ucv_array_sort(uc_value_t *uv, int (*cmp)(const void *, const void *)) { uc_array_t *array = ucv_as_array(uv); - if (ucv_type(uv) != UC_ARRAY || array->count <= 1) + if (ucv_type(uv) != UC_ARRAY || ucv_is_constant(uv) || array->count <= 1) return; + ucv_set_constant(uv, true); qsort(array->entries, array->count, sizeof(array->entries[0]), cmp); + ucv_set_constant(uv, false); } bool @@ -883,7 +888,7 @@ ucv_array_delete(uc_value_t *uv, size_t offset, size_t count) uc_array_t *array = ucv_as_array(uv); size_t i; - if (ucv_type(uv) != UC_ARRAY || array->count == 0) + if (ucv_type(uv) != UC_ARRAY || ucv_is_constant(uv) || array->count == 0) return false; if (offset >= array->count) @@ -913,7 +918,7 @@ ucv_array_set(uc_value_t *uv, size_t index, uc_value_t *item) { uc_array_t *array = ucv_as_array(uv); - if (ucv_type(uv) != UC_ARRAY) + if (ucv_type(uv) != UC_ARRAY || ucv_is_constant(uv)) return false; if (index >= array->count) { @@ -1100,7 +1105,8 @@ ucv_object_sort_common(uc_value_t *uv, object_sort_ctx_t *ctx) size_t count; } keys = { 0 }; - if (ucv_type(uv) != UC_OBJECT || lh_table_length(object->table) <= 1) + if (ucv_type(uv) != UC_OBJECT || ucv_is_constant(uv) || + lh_table_length(object->table) <= 1) return; for (t = object->table, e = t->head; e; e = e->next) @@ -1109,6 +1115,8 @@ ucv_object_sort_common(uc_value_t *uv, object_sort_ctx_t *ctx) if (!keys.entries) return; + /* Keep the saved hash table entry pointers valid until relinking. */ + ucv_set_constant(uv, true); uc_vector_sort(&keys, ctx->cmpr ? ucv_object_sort_r_cb : ucv_object_sort_cb, ctx); @@ -1128,6 +1136,7 @@ ucv_object_sort_common(uc_value_t *uv, object_sort_ctx_t *ctx) } uc_vector_clear(&keys); + ucv_set_constant(uv, false); } void