From 527864dd4e73d34df7d85cf1bb5e71ac5bafc020 Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 15:35:29 +0200 Subject: [PATCH 01/48] Fix for crash involving the use of private types used as default values This attempts to fix issues #4130 and #4153. The issue was when a private type in another package was used as a default value in a method call. Fix to #4130 Since it has been decided to treat this as a bug instead of a missing error, this PR implements the fix suggested by @ergl, namely using `lookup_try()` instead of lookup() in call.c's `check_partial_function_call()` since it allows to permit private types. Fix to #4153 This is also a simply fix to lookup.c that prevents a potential segfault by a dereferencing of `opt` (`typecheck_t* t = &opt->check;`) *before* `opt != NULL` was checked. As pointed out by @SeanTAllen, opt should not be NULL to begin with when lookup_nominal is called, and instead, an assert should be added and the NULL checks in that function removed. --- src/libponyc/type/lookup.c | 43 ++++++++++++++++++-------------------- src/libponyc/verify/call.c | 4 ++-- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/src/libponyc/type/lookup.c b/src/libponyc/type/lookup.c index f0b75bda08..03ad5c4190 100644 --- a/src/libponyc/type/lookup.c +++ b/src/libponyc/type/lookup.c @@ -57,14 +57,14 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, ast_t* orig, ast_t* type, const char* name, bool errors, bool allow_private) { pony_assert(ast_id(type) == TK_NOMINAL); + pony_assert(opt != NULL); typecheck_t* t = &opt->check; ast_t* def = (ast_t*)ast_data(type); AST_GET_CHILDREN(def, type_id, typeparams); const char* type_name = ast_name(type_id); - if(is_name_private(type_name) && (from != NULL) && (opt != NULL) - && !allow_private) + if(is_name_private(type_name) && (from != NULL) && !allow_private) { if(ast_nearest(def, TK_PACKAGE) != t->frame->package) { @@ -94,34 +94,31 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, case TK_FUN: { // Typecheck default args immediately. - if(opt != NULL) + AST_GET_CHILDREN(find, cap, id, typeparams, params); + ast_t* param = ast_child(params); + + while(param != NULL) { - AST_GET_CHILDREN(find, cap, id, typeparams, params); - ast_t* param = ast_child(params); + AST_GET_CHILDREN(param, name, type, def_arg); - while(param != NULL) + if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) { - AST_GET_CHILDREN(param, name, type, def_arg); - - if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) - { - ast_t* child = ast_child(def_arg); + ast_t* child = ast_child(def_arg); - if(ast_id(child) == TK_CALL) - ast_settype(child, ast_from(child, TK_INFERTYPE)); + if(ast_id(child) == TK_CALL) + ast_settype(child, ast_from(child, TK_INFERTYPE)); - if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, - PASS_EXPR) != AST_OK) - return NULL; + if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, + PASS_EXPR) != AST_OK) + return NULL; - def_arg = ast_childidx(param, 2); + def_arg = ast_childidx(param, 2); - if(!coerce_literals(&def_arg, type, opt)) - return NULL; - } - - param = ast_sibling(param); + if(!coerce_literals(&def_arg, type, opt)) + return NULL; } + + param = ast_sibling(param); } break; } @@ -140,7 +137,7 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, return NULL; } - if(is_name_private(name) && (from != NULL) && (opt != NULL) && !allow_private) + if(is_name_private(name) && (from != NULL) && !allow_private) { switch(ast_id(find)) { diff --git a/src/libponyc/verify/call.c b/src/libponyc/verify/call.c index 814d60e25d..c820819f9d 100644 --- a/src/libponyc/verify/call.c +++ b/src/libponyc/verify/call.c @@ -28,8 +28,8 @@ static bool check_partial_function_call(pass_opt_t* opt, ast_t* ast) ast_id(call_error) == TK_NONE || ast_id(call_error) == TK_DONTCARE); // Look up the original method definition for this method call. - deferred_reification_t* method_def = lookup(opt, receiver, ast_type(receiver), - ast_name(method)); + deferred_reification_t* method_def = lookup_try(opt, receiver, ast_type(receiver), + ast_name(method), true); // allow private types ast_t* method_ast = method_def->ast; deferred_reify_free(method_def); From 0eae6eff89903a1dc7f7404a704d822ab66c5824 Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 15:59:53 +0200 Subject: [PATCH 02/48] Revert "Fix for crash involving the use of private types used as default values" This reverts commit 527864dd4e73d34df7d85cf1bb5e71ac5bafc020. --- src/libponyc/type/lookup.c | 43 ++++++++++++++++++++------------------ src/libponyc/verify/call.c | 4 ++-- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/libponyc/type/lookup.c b/src/libponyc/type/lookup.c index 03ad5c4190..f0b75bda08 100644 --- a/src/libponyc/type/lookup.c +++ b/src/libponyc/type/lookup.c @@ -57,14 +57,14 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, ast_t* orig, ast_t* type, const char* name, bool errors, bool allow_private) { pony_assert(ast_id(type) == TK_NOMINAL); - pony_assert(opt != NULL); typecheck_t* t = &opt->check; ast_t* def = (ast_t*)ast_data(type); AST_GET_CHILDREN(def, type_id, typeparams); const char* type_name = ast_name(type_id); - if(is_name_private(type_name) && (from != NULL) && !allow_private) + if(is_name_private(type_name) && (from != NULL) && (opt != NULL) + && !allow_private) { if(ast_nearest(def, TK_PACKAGE) != t->frame->package) { @@ -94,31 +94,34 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, case TK_FUN: { // Typecheck default args immediately. - AST_GET_CHILDREN(find, cap, id, typeparams, params); - ast_t* param = ast_child(params); - - while(param != NULL) + if(opt != NULL) { - AST_GET_CHILDREN(param, name, type, def_arg); + AST_GET_CHILDREN(find, cap, id, typeparams, params); + ast_t* param = ast_child(params); - if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) + while(param != NULL) { - ast_t* child = ast_child(def_arg); + AST_GET_CHILDREN(param, name, type, def_arg); - if(ast_id(child) == TK_CALL) - ast_settype(child, ast_from(child, TK_INFERTYPE)); + if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) + { + ast_t* child = ast_child(def_arg); - if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, - PASS_EXPR) != AST_OK) - return NULL; + if(ast_id(child) == TK_CALL) + ast_settype(child, ast_from(child, TK_INFERTYPE)); - def_arg = ast_childidx(param, 2); + if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, + PASS_EXPR) != AST_OK) + return NULL; - if(!coerce_literals(&def_arg, type, opt)) - return NULL; - } + def_arg = ast_childidx(param, 2); - param = ast_sibling(param); + if(!coerce_literals(&def_arg, type, opt)) + return NULL; + } + + param = ast_sibling(param); + } } break; } @@ -137,7 +140,7 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, return NULL; } - if(is_name_private(name) && (from != NULL) && !allow_private) + if(is_name_private(name) && (from != NULL) && (opt != NULL) && !allow_private) { switch(ast_id(find)) { diff --git a/src/libponyc/verify/call.c b/src/libponyc/verify/call.c index c820819f9d..814d60e25d 100644 --- a/src/libponyc/verify/call.c +++ b/src/libponyc/verify/call.c @@ -28,8 +28,8 @@ static bool check_partial_function_call(pass_opt_t* opt, ast_t* ast) ast_id(call_error) == TK_NONE || ast_id(call_error) == TK_DONTCARE); // Look up the original method definition for this method call. - deferred_reification_t* method_def = lookup_try(opt, receiver, ast_type(receiver), - ast_name(method), true); // allow private types + deferred_reification_t* method_def = lookup(opt, receiver, ast_type(receiver), + ast_name(method)); ast_t* method_ast = method_def->ast; deferred_reify_free(method_def); From 7c18b7f49b85c114a923980443fe7c5f3bbb5c34 Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 16:06:46 +0200 Subject: [PATCH 03/48] Fix for crash involving the use of private types used as default values This attempts to fix #4130 This crash stems from the use of a private type as defined in another package when it was used as a default value in a method call. Since it has been decided to treat this as a bug instead of a missing error, this PR implements the fix suggested by @ergl, namely using `lookup_try()` instead of lookup() in call.c's `check_partial_function_call()` since the former can be configured to permit private types. Fix to #4153 This is a simply change to `lookup_nominal()` in lookup.c that prevents a potential segfault by a dereferencing of `opt` (`typecheck_t* t = &opt->check;`) *before* `opt != NULL` was checked. As pointed out by @SeanTAllen, opt should not be NULL to begin with when `lookup_nominal()` is called, and instead, an assert should be added and the NULL checks in that function removed. With this PR, the two examples below that crashed the compiler now both compile: Original example: ```pony // inside the "useful" package primitive _PrivateDefault actor Useful[A: Any val] fun tag config(value: (A | _PrivateDefault) = _PrivateDefault): Useful[A] => this // inside "main" use "useful" primitive This primitive That type Stuff is (This | That) actor Main new create(env: Env) => let u = Useful[Stuff].config() ``` Minimal example: ```pony // In the "lib" pacakge primitive _Private primitive Public fun apply[T](v: (T | _Private) = _Private): None => None // In main use lib = "lib" actor Main new create(env: Env) => let p = lib.Public.apply[U8]() env.out.print(p.string()) ``` --- src/libponyc/verify/call.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libponyc/verify/call.c b/src/libponyc/verify/call.c index 814d60e25d..c820819f9d 100644 --- a/src/libponyc/verify/call.c +++ b/src/libponyc/verify/call.c @@ -28,8 +28,8 @@ static bool check_partial_function_call(pass_opt_t* opt, ast_t* ast) ast_id(call_error) == TK_NONE || ast_id(call_error) == TK_DONTCARE); // Look up the original method definition for this method call. - deferred_reification_t* method_def = lookup(opt, receiver, ast_type(receiver), - ast_name(method)); + deferred_reification_t* method_def = lookup_try(opt, receiver, ast_type(receiver), + ast_name(method), true); // allow private types ast_t* method_ast = method_def->ast; deferred_reify_free(method_def); From 6391765e1fc18f2caded55015a45be70abfbd574 Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 18:32:08 +0200 Subject: [PATCH 04/48] Adding test runner --- .../lib/lib.pony | 4 ++++ .../main.pony | 6 ++++++ 2 files changed, 10 insertions(+) create mode 100644 test/libponyc-run/private-type-as-default-argument-in-public-function/lib/lib.pony create mode 100644 test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony diff --git a/test/libponyc-run/private-type-as-default-argument-in-public-function/lib/lib.pony b/test/libponyc-run/private-type-as-default-argument-in-public-function/lib/lib.pony new file mode 100644 index 0000000000..4bcad15e24 --- /dev/null +++ b/test/libponyc-run/private-type-as-default-argument-in-public-function/lib/lib.pony @@ -0,0 +1,4 @@ +primitive _Private + +primitive Public + fun apply[T](v: (T | _Private) = _Private): None => None diff --git a/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony b/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony new file mode 100644 index 0000000000..44eede2517 --- /dev/null +++ b/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony @@ -0,0 +1,6 @@ +use lib = "lib" + +actor Main + new create(env: Env) => + let p = lib.Public.apply[U8]() + env.out.print(p.string()) From 5fd6c26f316c0f0a7de4ef18b8bee205b768d60e Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 27 Jul 2022 21:10:39 +0200 Subject: [PATCH 05/48] Create 4167.md Adding release notes --- .release-notes/4167.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .release-notes/4167.md diff --git a/.release-notes/4167.md b/.release-notes/4167.md new file mode 100644 index 0000000000..a7da9e8cc2 --- /dev/null +++ b/.release-notes/4167.md @@ -0,0 +1,22 @@ +## Fix for compiler segfault #4130 + +The compiler crash described in #4130 happened with code that attempted to call a method in a different package when this remote method used a package-private type as a default parameter. + +```pony +// In the "lib" pacakge + +primitive _Private + +primitive Public + fun apply[T](v: (T | _Private) = _Private): None => None + +// In main +use lib = "lib" + +actor Main + new create(env: Env) => + let p = lib.Public.apply[U8]() + env.out.print(p.string()) +``` + +It was decided that this code is valid Pony code, and this PR fixes the crash so that code like the above compiles. From cb3b9601815391e43096d623a1eb85037c585d8e Mon Sep 17 00:00:00 2001 From: stefandd Date: Thu, 28 Jul 2022 17:51:29 +0200 Subject: [PATCH 06/48] Update test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony Co-authored-by: Borja o'Cook --- .../main.pony | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony b/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony index 44eede2517..41c4a74d2e 100644 --- a/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony +++ b/test/libponyc-run/private-type-as-default-argument-in-public-function/main.pony @@ -2,5 +2,4 @@ use lib = "lib" actor Main new create(env: Env) => - let p = lib.Public.apply[U8]() - env.out.print(p.string()) + lib.Public.apply[U8]() From 323e371f109ea2f839f389296e1e0a7fc9a4b6db Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 30 Jul 2022 20:26:19 +0200 Subject: [PATCH 07/48] Update 4167.md --- .release-notes/4167.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-notes/4167.md b/.release-notes/4167.md index a7da9e8cc2..a95170d32c 100644 --- a/.release-notes/4167.md +++ b/.release-notes/4167.md @@ -1,4 +1,4 @@ -## Fix for compiler segfault #4130 +## Fix compiler crash related to using private types as default arguments The compiler crash described in #4130 happened with code that attempted to call a method in a different package when this remote method used a package-private type as a default parameter. From a7c6399dee64a712f8a9824ff718af4e330716b6 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 6 Aug 2022 22:07:40 +0200 Subject: [PATCH 08/48] Preliminary fix to segfault in Array's .to_copy() --- packages/builtin/array.pony | 12 +++++++----- .../expected-exit-code.txt | 1 + .../main.pony | 14 ++++++++++++++ 3 files changed, 22 insertions(+), 5 deletions(-) create mode 100644 test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/expected-exit-code.txt create mode 100644 test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/main.pony diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index d5b25c6ce3..4ed03a2218 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -552,13 +552,15 @@ class Array[A] is Seq[A] """ Copy len elements from this(src_idx) to dst(dst_idx). """ - dst.reserve(dst_idx + len) - _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), len) + if _alloc > 0 then + dst.reserve(dst_idx + len) + _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), len) - if dst._size < (dst_idx + len) then - dst._size = dst_idx + len + if dst._size < (dst_idx + len) then + dst._size = dst_idx + len + end end - + fun ref remove(i: USize, n: USize) => """ Remove n elements from the array, beginning at index i. diff --git a/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/expected-exit-code.txt b/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/expected-exit-code.txt new file mode 100644 index 0000000000..56a6051ca2 --- /dev/null +++ b/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/expected-exit-code.txt @@ -0,0 +1 @@ +1 \ No newline at end of file diff --git a/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/main.pony b/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/main.pony new file mode 100644 index 0000000000..f931fcf496 --- /dev/null +++ b/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/main.pony @@ -0,0 +1,14 @@ +use @pony_exitcode[None](code: I32) + +class Foo + embed _array: Array[I32] = [] + +new create() => None +new clone(src: Foo) => src._array.copy_to(_array, 0, 0, 10) + +actor Main + + new create(env': Env) => + let f1 = Foo + let f2 = Foo.clone(f1) + @pony_exitcode(1) From 0f0afed414f9578d7f4dea6b04ae71fcee9bbf55 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 6 Aug 2022 22:31:04 +0200 Subject: [PATCH 09/48] Update array.pony --- packages/builtin/array.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index 4ed03a2218..fcbc13dfb7 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -560,7 +560,7 @@ class Array[A] is Seq[A] dst._size = dst_idx + len end end - + fun ref remove(i: USize, n: USize) => """ Remove n elements from the array, beginning at index i. From 200baeaeaaccbfe4ff8f8fbf522abed9246961f1 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 6 Aug 2022 22:32:02 +0200 Subject: [PATCH 10/48] Update array.pony --- packages/builtin/array.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index fcbc13dfb7..bb0d968edd 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -560,7 +560,7 @@ class Array[A] is Seq[A] dst._size = dst_idx + len end end - + fun ref remove(i: USize, n: USize) => """ Remove n elements from the array, beginning at index i. From e4c5eae12347e2ef800e6807213c79e64dc01671 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 6 Aug 2022 22:42:14 +0200 Subject: [PATCH 11/48] Create 4173.md Add release notes for #4173 --- .release-notes/4173.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 .release-notes/4173.md diff --git a/.release-notes/4173.md b/.release-notes/4173.md new file mode 100644 index 0000000000..22dd0fdc6d --- /dev/null +++ b/.release-notes/4173.md @@ -0,0 +1,18 @@ +## Fix for segfault when using Array.copy_to() with an empty Array + +Currently, the `Array.copy_to()` function does not check whether the Array is empty or has been instanced with any values. This leads to a runtime crash when calling `copy_to()` on an empty array such as in this example (issue #4172): + +```pony +class Foo + embed _array: Array[I32] = [] + +new create() => None +new clone(src: Foo) => src._array.copy_to(_array, 0, 0, 10) + +actor Main + + new create(env': Env) => + let f2 = Foo.clone(Foo) +``` + +This PR is a preliminary fix that safeguards for such cases -- calling `copy_to()` on an empty Array will simply do nothing. From 43b1910b5f9c3704fa657e82fb5d42c3572ff131 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 6 Aug 2022 23:51:56 +0200 Subject: [PATCH 12/48] Replace runner test with builtin_test --- packages/builtin_test/_test.pony | 16 ++++++++++++++++ .../expected-exit-code.txt | 1 - .../main.pony | 14 -------------- 3 files changed, 16 insertions(+), 15 deletions(-) delete mode 100644 test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/expected-exit-code.txt delete mode 100644 test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/main.pony diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 5ac260fd8d..a78783dc15 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -30,6 +30,7 @@ actor \nodoc\ Main is TestList test(_TestArrayConcat) test(_TestArrayFind) test(_TestArrayFromCPointer) + test(_TestArrayCopyTo) test(_TestArrayInsert) test(_TestArraySlice) test(_TestArraySwapElements) @@ -1764,6 +1765,21 @@ class \nodoc\ iso _TestArrayFromCPointer is UnitTest let arr = Array[U8].from_cpointer(Pointer[U8], 1, 1) h.assert_eq[USize](0, arr.size()) +class \nodoc\ iso _TestArrayCopyTo is UnitTest + """ + Test that .copy_to() on an empty Array does not change the target + + Verifies we don't get a segfault + https://github.com/ponylang/ponyc/issues/4172 + """ + fun name(): String => "builtin/Array.copy_to" + + fun apply(h: TestHelper) => + let a: Array[U8] = [] + let b: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + a.copy_to(b, 0, 0, 10) // attempt to copy 10 elements from the empty array + h.assert_array_eq[U8](b, [0; 1; 2; 3; 4; 5; 6]) + class \nodoc\ iso _TestMath128 is UnitTest """ Test 128 bit integer math. diff --git a/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/expected-exit-code.txt b/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/expected-exit-code.txt deleted file mode 100644 index 56a6051ca2..0000000000 --- a/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/expected-exit-code.txt +++ /dev/null @@ -1 +0,0 @@ -1 \ No newline at end of file diff --git a/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/main.pony b/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/main.pony deleted file mode 100644 index f931fcf496..0000000000 --- a/test/libponyc-run/array-no-crash-in-copy-to-from-empty-array/main.pony +++ /dev/null @@ -1,14 +0,0 @@ -use @pony_exitcode[None](code: I32) - -class Foo - embed _array: Array[I32] = [] - -new create() => None -new clone(src: Foo) => src._array.copy_to(_array, 0, 0, 10) - -actor Main - - new create(env': Env) => - let f1 = Foo - let f2 = Foo.clone(f1) - @pony_exitcode(1) From dc2758e7a2dd044e99e60e592128ac0a75259202 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:30:14 +0200 Subject: [PATCH 13/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index 22dd0fdc6d..91dd1bd8b9 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -15,4 +15,3 @@ actor Main let f2 = Foo.clone(Foo) ``` -This PR is a preliminary fix that safeguards for such cases -- calling `copy_to()` on an empty Array will simply do nothing. From 6145521d80984c0a01e759cf3a48f1e010a31d9e Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:30:20 +0200 Subject: [PATCH 14/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index 91dd1bd8b9..d21d77aed9 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -1,6 +1,6 @@ ## Fix for segfault when using Array.copy_to() with an empty Array -Currently, the `Array.copy_to()` function does not check whether the Array is empty or has been instanced with any values. This leads to a runtime crash when calling `copy_to()` on an empty array such as in this example (issue #4172): +Previously, `Array.copy_to()` did not check whether the source Array had been initialized. Uninitialized arrays internally have a null pointer for their data as no memory has been allocated for them yet. When doing a copy like in the example below, this would result in an attempt to dereference the null pointer and a segfault. ```pony class Foo From 79cfe22f8884f9694e693ba36ce8b3631c5015b4 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:30:33 +0200 Subject: [PATCH 15/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index d21d77aed9..54a04df8ff 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -4,7 +4,7 @@ Previously, `Array.copy_to()` did not check whether the source Array had been in ```pony class Foo - embed _array: Array[I32] = [] + embed _array: Array[I32] = [] new create() => None new clone(src: Foo) => src._array.copy_to(_array, 0, 0, 10) From 5d54c0bb0ef48ecc0d34cefaeeedc07a0e458d74 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:30:40 +0200 Subject: [PATCH 16/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index 54a04df8ff..c022f41963 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -6,7 +6,9 @@ Previously, `Array.copy_to()` did not check whether the source Array had been in class Foo embed _array: Array[I32] = [] -new create() => None + new create() => + None + new clone(src: Foo) => src._array.copy_to(_array, 0, 0, 10) actor Main From fca55f36fbb845113a154d1c4907b79d60e2f96c Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:30:49 +0200 Subject: [PATCH 17/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index c022f41963..023def9dbc 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -9,7 +9,8 @@ class Foo new create() => None -new clone(src: Foo) => src._array.copy_to(_array, 0, 0, 10) + new clone(src: Foo) => + src._array.copy_to(_array, 0, 0, 10) actor Main From 20bd4780cc42d22d9e69e3a4e19133cee65af0e3 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:30:59 +0200 Subject: [PATCH 18/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index 023def9dbc..28d1e55286 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -13,7 +13,6 @@ class Foo src._array.copy_to(_array, 0, 0, 10) actor Main - new create(env': Env) => let f2 = Foo.clone(Foo) ``` From 9646618533c60c5e32753172bcfe9b8b84281304 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:31:04 +0200 Subject: [PATCH 19/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index 28d1e55286..7e6b6fbc48 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -13,7 +13,7 @@ class Foo src._array.copy_to(_array, 0, 0, 10) actor Main - new create(env': Env) => + new create(env': Env) => let f2 = Foo.clone(Foo) ``` From 6d19d5aa52a1af567d1e2cc32602aa67cdffc608 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:31:11 +0200 Subject: [PATCH 20/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index 7e6b6fbc48..eb511074f4 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -14,6 +14,7 @@ class Foo actor Main new create(env': Env) => - let f2 = Foo.clone(Foo) + let foo1 = Foo + let foo2 = Foo.clone(foo1) ``` From d3a9a70d7cf6e0be83f0fef86138438140fb0fb6 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:31:32 +0200 Subject: [PATCH 21/48] Update .release-notes/4173.md Co-authored-by: Sean T Allen --- .release-notes/4173.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index eb511074f4..3d078dfcfe 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -18,3 +18,6 @@ actor Main let foo2 = Foo.clone(foo1) ``` +`Array` is part of the `builtin` package of the standard library. Only code in `builtin` is allowed to do direct pointer manipulations like those that `Array` uses to implement `copy_to`. Direct pointer manipulations, if done incorrectly, can lead to segfaults and other memory related errors. Because pointer manipulation can be dangerous, only code in the `builtin` package is allowed to do it. + +We consider unsafe pointer operations in the `builtin` package to be of utmost importance and once this bug was found moved quickly to release a fix. From 3e80b98ee85282d32a51d4400e9ba3c8ebf5c0ca Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:31:39 +0200 Subject: [PATCH 22/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index a78783dc15..671aad2393 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1772,7 +1772,7 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest Verifies we don't get a segfault https://github.com/ponylang/ponyc/issues/4172 """ - fun name(): String => "builtin/Array.copy_to" + fun name(): String => "builtin/Array.copy_to_with_uninitialized_source" fun apply(h: TestHelper) => let a: Array[U8] = [] From 4a8b13a837c67175ead1b89d882bf6342632561b Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:31:45 +0200 Subject: [PATCH 23/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 671aad2393..267a017c66 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1766,12 +1766,6 @@ class \nodoc\ iso _TestArrayFromCPointer is UnitTest h.assert_eq[USize](0, arr.size()) class \nodoc\ iso _TestArrayCopyTo is UnitTest - """ - Test that .copy_to() on an empty Array does not change the target - - Verifies we don't get a segfault - https://github.com/ponylang/ponyc/issues/4172 - """ fun name(): String => "builtin/Array.copy_to_with_uninitialized_source" fun apply(h: TestHelper) => From 1545c6ea7a14fab77eb6e2c82e7bb0e1c4599369 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:31:51 +0200 Subject: [PATCH 24/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 267a017c66..99074ff0a1 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1770,7 +1770,7 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest fun apply(h: TestHelper) => let a: Array[U8] = [] - let b: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + let dest1: Array[U8] = [0; 1; 2; 3; 4; 5; 6] a.copy_to(b, 0, 0, 10) // attempt to copy 10 elements from the empty array h.assert_array_eq[U8](b, [0; 1; 2; 3; 4; 5; 6]) From 882059c6d399a2b0e85452a99b806d532b9de823 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:31:59 +0200 Subject: [PATCH 25/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 99074ff0a1..5a1cb67797 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1769,7 +1769,8 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest fun name(): String => "builtin/Array.copy_to_with_uninitialized_source" fun apply(h: TestHelper) => - let a: Array[U8] = [] + // Test that a using an uninitialized array as a source leaves the destination unchanged + let src1: Array[U8] = [] let dest1: Array[U8] = [0; 1; 2; 3; 4; 5; 6] a.copy_to(b, 0, 0, 10) // attempt to copy 10 elements from the empty array h.assert_array_eq[U8](b, [0; 1; 2; 3; 4; 5; 6]) From 533fb922cbf04d82bd9695d5d5ce5ea7039d2e12 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:32:05 +0200 Subject: [PATCH 26/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 5a1cb67797..80359f96ad 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1772,7 +1772,7 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest // Test that a using an uninitialized array as a source leaves the destination unchanged let src1: Array[U8] = [] let dest1: Array[U8] = [0; 1; 2; 3; 4; 5; 6] - a.copy_to(b, 0, 0, 10) // attempt to copy 10 elements from the empty array + src1.copy_to(dest1, 0, 0, 10) h.assert_array_eq[U8](b, [0; 1; 2; 3; 4; 5; 6]) class \nodoc\ iso _TestMath128 is UnitTest From 65be1ce4499b51efbaac77e0eca139b9e4d53b45 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:32:10 +0200 Subject: [PATCH 27/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 80359f96ad..7b18f0d483 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1773,7 +1773,7 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest let src1: Array[U8] = [] let dest1: Array[U8] = [0; 1; 2; 3; 4; 5; 6] src1.copy_to(dest1, 0, 0, 10) - h.assert_array_eq[U8](b, [0; 1; 2; 3; 4; 5; 6]) + h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest1) class \nodoc\ iso _TestMath128 is UnitTest """ From 136ff80e2413e566630e97187c4fda6de1e06308 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:32:18 +0200 Subject: [PATCH 28/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 7b18f0d483..b2205613ef 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1775,6 +1775,19 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest src1.copy_to(dest1, 0, 0, 10) h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest1) + // Test that copying from an empty source array leaves + // the destination unchanged + try + // create an initialized array and then remove it's only element + // leaving it empty + let src2: Array[U8] = [1] + src2.pop()? + let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + src2.copy_to(dest2, 0, 0, 10) + end + + h.assert_array_eq([0; 1; 2; 3; 4; 5; 6], dest2) + class \nodoc\ iso _TestMath128 is UnitTest """ Test 128 bit integer math. From 9ee68fa1a0db9a401e968b27503f74b060cd99c9 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:37:51 +0200 Subject: [PATCH 29/48] Update _test.pony --- packages/builtin_test/_test.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index b2205613ef..e776337bd4 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1777,12 +1777,12 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest // Test that copying from an empty source array leaves // the destination unchanged + let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] try // create an initialized array and then remove it's only element // leaving it empty let src2: Array[U8] = [1] src2.pop()? - let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] src2.copy_to(dest2, 0, 0, 10) end From 12d54c920a9f72d39a43f6c92062f31e0d6eed4e Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 16:49:35 +0200 Subject: [PATCH 30/48] Adjust the fix to work for both uninitialized and empty arrays + Test fixes --- packages/builtin/array.pony | 2 +- packages/builtin_test/_test.pony | 19 ++++++++----------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index bb0d968edd..1397b2d5cc 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -552,7 +552,7 @@ class Array[A] is Seq[A] """ Copy len elements from this(src_idx) to dst(dst_idx). """ - if _alloc > 0 then + if _size > 0 then dst.reserve(dst_idx + len) _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), len) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index e776337bd4..18ab374317 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1774,20 +1774,17 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest let dest1: Array[U8] = [0; 1; 2; 3; 4; 5; 6] src1.copy_to(dest1, 0, 0, 10) h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest1) - // Test that copying from an empty source array leaves // the destination unchanged let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] - try - // create an initialized array and then remove it's only element - // leaving it empty - let src2: Array[U8] = [1] - src2.pop()? - src2.copy_to(dest2, 0, 0, 10) - end - - h.assert_array_eq([0; 1; 2; 3; 4; 5; 6], dest2) - + // create an initialized array and then remove it's only element + // leaving it empty + let src2: Array[U8] = [1] + try src2.pop()? end + h.assert_eq[USize](0, src2.size()) + src2.copy_to(dest2, 0, 0, 10) + h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2) + class \nodoc\ iso _TestMath128 is UnitTest """ Test 128 bit integer math. From b929676a727c8c07c519e696c41b0f38cd29dca4 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 18:03:06 +0200 Subject: [PATCH 31/48] Update array.pony Update fix to include #4174 --- packages/builtin/array.pony | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index 1397b2d5cc..acc8c6d76d 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -552,12 +552,13 @@ class Array[A] is Seq[A] """ Copy len elements from this(src_idx) to dst(dst_idx). """ - if _size > 0 then - dst.reserve(dst_idx + len) - _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), len) + let copy_size = len.min(_size) // make sure we don't try to copy more than _size elements + if copy_size > 0 then + dst.reserve(dst_idx + copy_size) + _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), copy_size) - if dst._size < (dst_idx + len) then - dst._size = dst_idx + len + if dst._size < (dst_idx + copy_size) then + dst._size = dst_idx + copy_size end end From e473f72fc05a67fb1f03273a66bc51f2011d6347 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 18:04:52 +0200 Subject: [PATCH 32/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 18ab374317..b2e153912b 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1774,6 +1774,7 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest let dest1: Array[U8] = [0; 1; 2; 3; 4; 5; 6] src1.copy_to(dest1, 0, 0, 10) h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest1) + // Test that copying from an empty source array leaves // the destination unchanged let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] From bed272f22ad3dcca17a0035ae1aba25e6478b639 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 18:05:24 +0200 Subject: [PATCH 33/48] Update packages/builtin_test/_test.pony Co-authored-by: Sean T Allen --- packages/builtin_test/_test.pony | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index b2e153912b..aa610b55f9 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1766,7 +1766,8 @@ class \nodoc\ iso _TestArrayFromCPointer is UnitTest h.assert_eq[USize](0, arr.size()) class \nodoc\ iso _TestArrayCopyTo is UnitTest - fun name(): String => "builtin/Array.copy_to_with_uninitialized_source" + fun name(): String => + "builtin/Array.copy_to" fun apply(h: TestHelper) => // Test that a using an uninitialized array as a source leaves the destination unchanged From 7daa2380c612cdba32a9c0e4aa24d61267ac5323 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 18:06:28 +0200 Subject: [PATCH 34/48] Update _test.pony --- packages/builtin_test/_test.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 18ab374317..127fe118d1 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1776,12 +1776,12 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest1) // Test that copying from an empty source array leaves // the destination unchanged - let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] // create an initialized array and then remove it's only element // leaving it empty let src2: Array[U8] = [1] try src2.pop()? end h.assert_eq[USize](0, src2.size()) + let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] src2.copy_to(dest2, 0, 0, 10) h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2) From 30e47dbf49cf6ee7bf829a20a7633bcb4a8eed95 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 18:17:04 +0200 Subject: [PATCH 35/48] Update array.pony --- packages/builtin/array.pony | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index acc8c6d76d..d498d41069 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -552,13 +552,13 @@ class Array[A] is Seq[A] """ Copy len elements from this(src_idx) to dst(dst_idx). """ - let copy_size = len.min(_size) // make sure we don't try to copy more than _size elements - if copy_size > 0 then - dst.reserve(dst_idx + copy_size) - _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), copy_size) + let count = len.min(_size - src_idx) + if count > 0 then + dst.reserve(dst_idx + count) + _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), count) - if dst._size < (dst_idx + copy_size) then - dst._size = dst_idx + copy_size + if dst._size < (dst_idx + count) then + dst._size = dst_idx + count end end From e2fa066fd2aadd56f01a0ffd02749a8165cbf0f6 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 18:54:14 +0200 Subject: [PATCH 36/48] Update _test.pony Include tests for #4174 --- packages/builtin_test/_test.pony | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index dda002b13e..23f4e94708 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1784,8 +1784,13 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest try src2.pop()? end h.assert_eq[USize](0, src2.size()) let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] - src2.copy_to(dest2, 0, 0, 10) + src2.copy_to(dest2, 0, 0, 10) // try to copy 9 non-existant elements h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2) + src2.copy_to(dest2, 11, 0, 1) // try to copy from too high start index + h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2) + src2.push(1) // re-add single element [1] + src2.copy_to(dest2, 0, 0, 10) // copies the sole available element + h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest2) class \nodoc\ iso _TestMath128 is UnitTest """ From 0d9099d62e783642f184940d68bef5b2385f7bf6 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 18:55:55 +0200 Subject: [PATCH 37/48] Delete 4167.md --- .release-notes/4167.md | 22 ---------------------- 1 file changed, 22 deletions(-) delete mode 100644 .release-notes/4167.md diff --git a/.release-notes/4167.md b/.release-notes/4167.md deleted file mode 100644 index a95170d32c..0000000000 --- a/.release-notes/4167.md +++ /dev/null @@ -1,22 +0,0 @@ -## Fix compiler crash related to using private types as default arguments - -The compiler crash described in #4130 happened with code that attempted to call a method in a different package when this remote method used a package-private type as a default parameter. - -```pony -// In the "lib" pacakge - -primitive _Private - -primitive Public - fun apply[T](v: (T | _Private) = _Private): None => None - -// In main -use lib = "lib" - -actor Main - new create(env: Env) => - let p = lib.Public.apply[U8]() - env.out.print(p.string()) -``` - -It was decided that this code is valid Pony code, and this PR fixes the crash so that code like the above compiles. From e83ea4c7835405a8ad0cb194283a972b2bc13de5 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 19:01:40 +0200 Subject: [PATCH 38/48] Update _test.pony Further small test improvement --- packages/builtin_test/_test.pony | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 23f4e94708..0c02819a88 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1784,11 +1784,11 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest try src2.pop()? end h.assert_eq[USize](0, src2.size()) let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] - src2.copy_to(dest2, 0, 0, 10) // try to copy 9 non-existant elements + src2.copy_to(dest2, 0, 0, 10) // try to copy 10 non-existant elements h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2) + src2.push(1) // re-add single element [1] src2.copy_to(dest2, 11, 0, 1) // try to copy from too high start index h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2) - src2.push(1) // re-add single element [1] src2.copy_to(dest2, 0, 0, 10) // copies the sole available element h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest2) From 1e2448c73d1e90f5899ccc34cc74696cdef627db Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 7 Aug 2022 19:25:51 +0200 Subject: [PATCH 39/48] Update array.pony Ouch -- USize overflow when `_size - src_index` < 0! --- packages/builtin/array.pony | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index d498d41069..e04cbddc32 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -552,13 +552,15 @@ class Array[A] is Seq[A] """ Copy len elements from this(src_idx) to dst(dst_idx). """ - let count = len.min(_size - src_idx) - if count > 0 then - dst.reserve(dst_idx + count) - _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), count) - - if dst._size < (dst_idx + count) then - dst._size = dst_idx + count + if src_idx < _size then + let count = len.min(_size - src_idx) + if count > 0 then + dst.reserve(dst_idx + count) + _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), count) + + if dst._size < (dst_idx + count) then + dst._size = dst_idx + count + end end end From 6c9b5eefb851ed621267d2affecd232c42158621 Mon Sep 17 00:00:00 2001 From: stefandd Date: Tue, 9 Aug 2022 12:15:11 +0200 Subject: [PATCH 40/48] Update _test.pony --- packages/builtin_test/_test.pony | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 0c02819a88..2b0bac548e 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -1778,8 +1778,6 @@ class \nodoc\ iso _TestArrayCopyTo is UnitTest // Test that copying from an empty source array leaves // the destination unchanged - // create an initialized array and then remove it's only element - // leaving it empty let src2: Array[U8] = [1] try src2.pop()? end h.assert_eq[USize](0, src2.size()) From 5f763f63783946e40641641edde1b5fc4d08950c Mon Sep 17 00:00:00 2001 From: stefandd Date: Thu, 11 Aug 2022 03:52:18 +0200 Subject: [PATCH 41/48] Update 4173.md --- .release-notes/4173.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index 3d078dfcfe..62ec5a75ac 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -1,6 +1,6 @@ -## Fix for segfault when using Array.copy_to() with an empty Array +## Fix for potential memory corruption in Array.copy_to() -Previously, `Array.copy_to()` did not check whether the source Array had been initialized. Uninitialized arrays internally have a null pointer for their data as no memory has been allocated for them yet. When doing a copy like in the example below, this would result in an attempt to dereference the null pointer and a segfault. +`Array.copy_to()` did not check whether the source Array had been initialized or if the requested number of elements to be copied exceeded the number of available elements (allocated memory). The issues would result in potential dereferencing of a null pointer or attempts to access unallocated memory. ```pony class Foo @@ -18,6 +18,6 @@ actor Main let foo2 = Foo.clone(foo1) ``` -`Array` is part of the `builtin` package of the standard library. Only code in `builtin` is allowed to do direct pointer manipulations like those that `Array` uses to implement `copy_to`. Direct pointer manipulations, if done incorrectly, can lead to segfaults and other memory related errors. Because pointer manipulation can be dangerous, only code in the `builtin` package is allowed to do it. +`Array` is part of the `builtin` package of the standard library. Only code in `builtin` is allowed to do direct pointer manipulations like those that `Array` uses to implement `copy_to`. Direct pointer manipulations, if done incorrectly, can lead to segfaults and other memory related errors. We consider unsafe pointer operations in the `builtin` package to be of utmost importance and once this bug was found moved quickly to release a fix. From c3ca05b756b05022632a1a1585388798e61ef98c Mon Sep 17 00:00:00 2001 From: stefandd Date: Wed, 17 Aug 2022 17:18:56 +0200 Subject: [PATCH 42/48] Further update to array.pony to fix #4174 --- packages/builtin/array.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index e04cbddc32..578ed8b819 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -552,7 +552,7 @@ class Array[A] is Seq[A] """ Copy len elements from this(src_idx) to dst(dst_idx). """ - if src_idx < _size then + if (src_idx < _size) and (dst_idx < dst._size) then let count = len.min(_size - src_idx) if count > 0 then dst.reserve(dst_idx + count) From f70d7b92fb4e8751bb6142651b109006aa4466fa Mon Sep 17 00:00:00 2001 From: stefandd Date: Mon, 22 Aug 2022 08:12:19 +0200 Subject: [PATCH 43/48] Update 4173.md --- .release-notes/4173.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-notes/4173.md b/.release-notes/4173.md index 62ec5a75ac..84ff547a28 100644 --- a/.release-notes/4173.md +++ b/.release-notes/4173.md @@ -1,6 +1,6 @@ ## Fix for potential memory corruption in Array.copy_to() -`Array.copy_to()` did not check whether the source Array had been initialized or if the requested number of elements to be copied exceeded the number of available elements (allocated memory). The issues would result in potential dereferencing of a null pointer or attempts to access unallocated memory. +`Array.copy_to()` did not check whether the source or destination Arrays had been initialized or whether the requested number of elements to be copied exceeded the number of available elements (allocated memory). These issues would result in potential dereferencing of a null pointer or attempts to access unallocated memory. ```pony class Foo From 3c9997990df0b84b0f9ba515a07f472c71b63067 Mon Sep 17 00:00:00 2001 From: stefandd Date: Mon, 22 Aug 2022 17:51:30 +0200 Subject: [PATCH 44/48] Update array.pony It is actually Ok if the `dst_idx == dst._size` (no gap) --- packages/builtin/array.pony | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index 578ed8b819..fe650670ee 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -552,7 +552,7 @@ class Array[A] is Seq[A] """ Copy len elements from this(src_idx) to dst(dst_idx). """ - if (src_idx < _size) and (dst_idx < dst._size) then + if (src_idx < _size) and (dst_idx <= dst._size) then let count = len.min(_size - src_idx) if count > 0 then dst.reserve(dst_idx + count) From 5b1542d9970b39c4991c2774c799a179ee497777 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 18 Sep 2022 06:09:28 +0200 Subject: [PATCH 45/48] Attempt to fix #4153 --- src/libponyc/reach/reach.c | 2 +- src/libponyc/type/lookup.c | 43 ++++++++++++++++++-------------------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/libponyc/reach/reach.c b/src/libponyc/reach/reach.c index a268840c6c..8f03c110ff 100644 --- a/src/libponyc/reach/reach.c +++ b/src/libponyc/reach/reach.c @@ -564,7 +564,7 @@ static void add_special(reach_t* r, reach_type_t* t, ast_t* type, const char* special, pass_opt_t* opt) { special = stringtab(special); - deferred_reification_t* find = lookup_try(NULL, NULL, type, special, false); + deferred_reification_t* find = lookup_try(opt, NULL, type, special, false); if(find != NULL) { diff --git a/src/libponyc/type/lookup.c b/src/libponyc/type/lookup.c index f0b75bda08..03ad5c4190 100644 --- a/src/libponyc/type/lookup.c +++ b/src/libponyc/type/lookup.c @@ -57,14 +57,14 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, ast_t* orig, ast_t* type, const char* name, bool errors, bool allow_private) { pony_assert(ast_id(type) == TK_NOMINAL); + pony_assert(opt != NULL); typecheck_t* t = &opt->check; ast_t* def = (ast_t*)ast_data(type); AST_GET_CHILDREN(def, type_id, typeparams); const char* type_name = ast_name(type_id); - if(is_name_private(type_name) && (from != NULL) && (opt != NULL) - && !allow_private) + if(is_name_private(type_name) && (from != NULL) && !allow_private) { if(ast_nearest(def, TK_PACKAGE) != t->frame->package) { @@ -94,34 +94,31 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, case TK_FUN: { // Typecheck default args immediately. - if(opt != NULL) + AST_GET_CHILDREN(find, cap, id, typeparams, params); + ast_t* param = ast_child(params); + + while(param != NULL) { - AST_GET_CHILDREN(find, cap, id, typeparams, params); - ast_t* param = ast_child(params); + AST_GET_CHILDREN(param, name, type, def_arg); - while(param != NULL) + if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) { - AST_GET_CHILDREN(param, name, type, def_arg); - - if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL)) - { - ast_t* child = ast_child(def_arg); + ast_t* child = ast_child(def_arg); - if(ast_id(child) == TK_CALL) - ast_settype(child, ast_from(child, TK_INFERTYPE)); + if(ast_id(child) == TK_CALL) + ast_settype(child, ast_from(child, TK_INFERTYPE)); - if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, - PASS_EXPR) != AST_OK) - return NULL; + if(ast_visit_scope(¶m, pass_pre_expr, pass_expr, opt, + PASS_EXPR) != AST_OK) + return NULL; - def_arg = ast_childidx(param, 2); + def_arg = ast_childidx(param, 2); - if(!coerce_literals(&def_arg, type, opt)) - return NULL; - } - - param = ast_sibling(param); + if(!coerce_literals(&def_arg, type, opt)) + return NULL; } + + param = ast_sibling(param); } break; } @@ -140,7 +137,7 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from, return NULL; } - if(is_name_private(name) && (from != NULL) && (opt != NULL) && !allow_private) + if(is_name_private(name) && (from != NULL) && !allow_private) { switch(ast_id(find)) { From 9f4f271b0791f46c6027d4e1ecec4e1ec219568a Mon Sep 17 00:00:00 2001 From: stefandd Date: Sun, 18 Sep 2022 06:18:19 +0200 Subject: [PATCH 46/48] Update reach.c --- src/libponyc/reach/reach.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libponyc/reach/reach.c b/src/libponyc/reach/reach.c index 8f03c110ff..a4885b890c 100644 --- a/src/libponyc/reach/reach.c +++ b/src/libponyc/reach/reach.c @@ -309,7 +309,7 @@ static void add_rmethod_to_subtypes(reach_t* r, reach_type_t* t, for(; child != NULL; child = ast_sibling(child)) { - deferred_reification_t* find = lookup_try(NULL, NULL, child, n->name, + deferred_reification_t* find = lookup_try(opt, NULL, child, n->name, false); if(find == NULL) @@ -349,7 +349,7 @@ static reach_method_t* add_rmethod(reach_t* r, reach_type_t* t, if(!internal) { ast_t* r_ast = set_cap_and_ephemeral(t->ast, cap, TK_NONE); - deferred_reification_t* fun = lookup(NULL, NULL, r_ast, n->name); + deferred_reification_t* fun = lookup(opt, NULL, r_ast, n->name); pony_assert(fun != NULL); // The typeargs and thistype are in the scope of r_ast but we're going to @@ -659,7 +659,7 @@ static void add_fields(reach_t* r, reach_type_t* t, pass_opt_t* opt) case TK_FLET: case TK_EMBED: { - deferred_reification_t* member_lookup = lookup(NULL, NULL, t->ast, + deferred_reification_t* member_lookup = lookup(opt, NULL, t->ast, ast_name(ast_child(member))); pony_assert(member_lookup != NULL); From bb41b029ea7ea228d3c36b796639a4292bd288d6 Mon Sep 17 00:00:00 2001 From: stefandd Date: Mon, 19 Sep 2022 11:23:37 +0200 Subject: [PATCH 47/48] Update reach.c --- src/libponyc/reach/reach.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libponyc/reach/reach.c b/src/libponyc/reach/reach.c index a4885b890c..31c04535fa 100644 --- a/src/libponyc/reach/reach.c +++ b/src/libponyc/reach/reach.c @@ -141,7 +141,7 @@ static reach_method_t* reach_rmethod(reach_method_name_t* n, const char* name) } static reach_method_name_t* add_method_name(reach_type_t* t, const char* name, - bool internal) + pass_opt_t* opt, bool internal) { reach_method_name_t* n = reach_method_name(t, name); @@ -159,7 +159,7 @@ static reach_method_name_t* add_method_name(reach_type_t* t, const char* name, n->cap = TK_BOX; n->internal = true; } else { - deferred_reification_t* fun = lookup(NULL, NULL, t->ast, name); + deferred_reification_t* fun = lookup(opt, NULL, t->ast, name); ast_t* fun_ast = fun->ast; n->id = ast_id(fun_ast); n->cap = ast_id(ast_child(fun_ast)); @@ -243,7 +243,7 @@ static void add_rmethod_to_subtype(reach_t* r, reach_type_t* t, reach_method_name_t* n, reach_method_t* m, pass_opt_t* opt) { // Add the method to the type if it isn't already there. - reach_method_name_t* n2 = add_method_name(t, n->name, false); + reach_method_name_t* n2 = add_method_name(t, n->name, opt, false); add_rmethod(r, t, n2, m->cap, m->typeargs, opt, false); // Add this mangling to the type if it isn't already there. @@ -414,7 +414,7 @@ static void add_internal(reach_t* r, reach_type_t* t, const char* name, pass_opt_t* opt) { name = stringtab(name); - reach_method_name_t* n = add_method_name(t, name, true); + reach_method_name_t* n = add_method_name(t, name, opt, true); add_rmethod(r, t, n, TK_BOX, NULL, opt, true); } @@ -695,7 +695,7 @@ static void add_fields(reach_t* r, reach_type_t* t, pass_opt_t* opt) if(!has_finaliser && needs_finaliser) { - reach_method_name_t* n = add_method_name(t, stringtab("_final"), true); + reach_method_name_t* n = add_method_name(t, stringtab("_final"), opt, true); add_rmethod(r, t, n, TK_BOX, NULL, opt, true); } } @@ -912,7 +912,7 @@ static reach_type_t* add_nominal(reach_t* r, ast_t* type, pass_opt_t* opt) AST_GET_CHILDREN(bare_method, cap, name, typeparams); pony_assert(ast_id(typeparams) == TK_NONE); - reach_method_name_t* n = add_method_name(t, ast_name(name), false); + reach_method_name_t* n = add_method_name(t, ast_name(name), opt, false); t->bare_method = add_rmethod(r, t, n, TK_AT, NULL, opt, false); } @@ -1323,7 +1323,7 @@ static void reachable_method(reach_t* r, deferred_reification_t* reify, t = add_type(r, type, opt); } - reach_method_name_t* n = add_method_name(t, name, false); + reach_method_name_t* n = add_method_name(t, name, opt, false); reach_method_t* m = add_rmethod(r, t, n, n->cap, typeargs, opt, false); if((n->id == TK_FUN) && ((n->cap == TK_BOX) || (n->cap == TK_TAG))) From 24cf1efd9eab28d6ee2dbe8001919523c5a4864b Mon Sep 17 00:00:00 2001 From: stefandd Date: Mon, 19 Sep 2022 13:18:54 +0200 Subject: [PATCH 48/48] Update genmatch.c --- src/libponyc/codegen/genmatch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libponyc/codegen/genmatch.c b/src/libponyc/codegen/genmatch.c index 4fc01f7119..dac80a711f 100644 --- a/src/libponyc/codegen/genmatch.c +++ b/src/libponyc/codegen/genmatch.c @@ -62,7 +62,7 @@ static ast_t* eq_param_type(compile_t* c, ast_t* pattern) { ast_t* pattern_type = deferred_reify(c->frame->reify, ast_type(pattern), c->opt); - deferred_reification_t* fun = lookup(NULL, pattern, pattern_type, c->str_eq); + deferred_reification_t* fun = lookup(c->opt, pattern, pattern_type, c->str_eq); AST_GET_CHILDREN(fun->ast, cap, id, typeparams, params, result, partial); ast_t* param = ast_child(params);