Fix memory safety of Array.copy_to
#4173
Conversation
This attempts to fix issues ponylang#4130 and ponylang#4153. The issue was when a private type in another package was used as a default value in a method call. Fix to ponylang#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 ponylang#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.
…ult values" This reverts commit 527864d.
This attempts to fix ponylang#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 ponylang#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()) ```
Adding release notes
…unction/main.pony Co-authored-by: Borja o'Cook <[email protected]>
ponylang-main
added
the
discuss during sync
|
The release notes appear to be for a different change. For a test, please add a unit test to builtin_tests not a runner test. |
SeanTAllen
added
the
changelog - fixed
The release notes are still the one from another PR I submitted for this release -- my branch was still based on release. I will submit release notes momentarily. Since this PR fixes a segfault, I assumed the test was supposed to show that's gone. However, I am not clear on how to test for the absence of a segfault outside a runner test. Could you give me some hints? |
Add release notes for ponylang#4173
|
A test that calling copy_to with an uninitialized array leaves the destination array unchanged is a sufficient test. |
|
@stefandd have you read the how to contribute doc? It has good pointer like "use a different branch for each PR" I'd suggest that you should at some point, start over with a new fork and never do anything with the main branch except track the upstream. |
So If I add such a test, you'd like me to remove the segfault-is-gone test? |
Yes please |
Will do. But for my understanding: why would such a test be sufficient? What if that test segfaulted? Is that being caught? I just built a builtin_test executable without the patch and it simply aborts. Is this tested in the CI? |
|
It doesn't matter if it segfaults. It matters that it does the right thing for a given test case. That it segfaults currently isn't of much interest. It matters that there's no test coverage to show it does the correct thing. If someone were to reintroduce the bug, the unit tests for the standard library would crash and we'd know something was wrong. There's plenty of ways that code could be wrong. So it's the "is it right" test that is important. There's compiler bugs that we need runner tests for and compiler features we need runner tests for. This isn't like that though. A good old unit test gives us all the coverage we need. |
|
Small nit. This isn't a bug with empty arrays. It's with uninitialized ones. If you initialized an array but let it empty, it wouldn't have segfaulted. |
|
This still has the extra release notes from a previous PR. |
Well this fixes both issues, but if you feel it is not ready to be included it is fine. If you change your mind, I'd like to ask you to adjust the release notes here to reflect both issues since you manage to be more concise. |
|
This is lacking test coverage to be accepted for 4174. The release notes should be updated, and it still has an extra release notes file that needs to be removed. If we moved forward with this fix rather than making partial, all of the above would need to be fixed and the PR title would need to be updated. |
Could I win you as a contributor for this? I attempted (did it work?) to make you a collaborator on the branch I worked from. I also changed the PR title - Ok so? I do not know how to remove leftover previous committs while the PR is open -- however I believed them to be benign since those committs were already merged with I added several tests to cover #4174 - let me know if you feel things are missing. I would suggest to edit the release-notes once concensus has been reached to move forward. |
Include tests for ponylang#4174
Further small test improvement
Ouch -- USize overflow when `_size - src_index` < 0!
SeanTAllen
changed the title
Array.copy_to in case of insufficient source elementsArray.copy_to
| 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) |
There was a problem hiding this comment.
I don't think this should be the desired behavior. I think if you give an out of range index it should be an error. This is a good point of conversation for the next sync call.
| let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] | ||
| 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.copy_to(dest2, 0, 0, 10) // copies the sole available element | ||
| h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest2) |
There was a problem hiding this comment.
for things like this, please put comments on their own line above the relevant code.
| 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.copy_to(dest2, 0, 0, 10) // copies the sole available element | ||
| h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest2) |
There was a problem hiding this comment.
each of these individual tests should have a newline between them and be prefaced by a comment explaining what is being tested.
|
Some happy path tests I would want to see:
|
| let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] | ||
| 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.copy_to(dest2, 0, 0, 10) // copies the sole available element | ||
| h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest2) |
There was a problem hiding this comment.
tests that rely on previous state are easy to break. for each test, please use new variables and initialize each test on its own. don't reuse data structures across the tests.
It is actually Ok if the `dst_idx == dst._size` (no gap)
ponylang-main
removed
the
discuss during sync
This fixes #4172 and #4174 by ensuring that only available elements get copied and that they are copied without any gap into the target array.
The reason for the crash in #4172 was that
_ptrcan beNULLwhen the Array is instanced as empty Array, e.g. letx: Array[I32] = [], whereas #4174 is more generally related to a request to copy more elements than there are or using a destination array index that creates a "gap" in the destination error.