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