ponyc
ponyc copied to clipboard
ponylang
Fix memory safety of `Array.copy_to`
This fixes #4172 and #4174 by ensuring that only available elements get copied. The reason for the crash in #4172 was that _ptr can be NULL when the Array is instanced as empty Array, e.g. let x: Array[I32] = [], whereas #4174 is more generally related to a request to copy more elements than there are.
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.
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.
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?
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.
A test that calling copy_to with an uninitialized array leaves the destination array unchanged is a sufficient test.
So If I add such a test, you'd like me to remove the segfault-is-gone test?
A test that calling copy_to with an uninitialized array leaves the destination array unchanged is a sufficient test.
So If I add such a test, you'd like me to remove the segfault-is-gone test?
Yes please
A test that calling copy_to with an uninitialized array leaves the destination array unchanged is a sufficient test.
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.
After this PR is merged, @stefandd, you are going to want to delete your fork and start over because the changes you'll need to make to remove the old release notes file is going to potentially cause issues in the future. But, not until after this PR is merged.
I made a number of suggestions that should address most issues. I also added an additional test for "empty" which wasn't covered. What was covered was "uninitialized" which is slightly different. I'm not sure that we have the second case correctly implemented.
@stefandd FYI, I've found other bugs in copy_to but I want to address them in other PRs.
After this PR is merged, @stefandd, you are going to want to delete your fork and start over because the changes you'll need to make to remove the old release notes file is going to potentially cause issues in the future. But, not until after this PR is merged.
Yes, I will do and will make it a habit to do so before every PR.
I made a number of suggestions that should address most issues. I also added an additional test for "empty" which wasn't covered. What was covered was "uninitialized" which is slightly different. I'm not sure that we have the second case correctly implemented.
Just submitted a small fix: the dest2 Array needed to be declared outside the try .. end for it to be in-scope for the assert.
I've realized that this is a subset of #4174. The issue isn't really that we are working with an initialized array, it's that "we are reading past the end".
If we were correctly checking that the length wasn't greater than the size then the alloc check wouldn't be needed at all as our size of 0 would preclude any copying.
I think that's a strong case for keeping the tests from this PR and adding them to the tests that are part of a larger fix for #4174.
Or rather taking what is being tested in terms of scenarios and verifying that an error is raised as the larger fix for #4174 makes it partial.
I've realized that this is a subset of #4174. The issue isn't really that we are working with an initialized array, it's that "we are reading past the end".
If we were correctly checking that the length wasn't greater than the size then the alloc check wouldn't be needed at all as our size of 0 would preclude any copying.
I think that's a strong case for keeping the tests from this PR and adding them to the tests that are part of a larger fix for #4174.
Or rather taking what is being tested in terms of scenarios and verifying that an error is raised as the larger fix for #4174 makes it partial.
@SeanTAllen I updated this to also fix #4174 -- could you please check?
@stefandd there's no passing tests that address everything noted that should be tested per #4173 to verify that such a fix is complete. Full tests of known problems are required at this point. I think we should leave this until more core team members way in on what the proper response to "bad input" should be. I'm strongly in favor of making copy_to partial.
@stefandd if we moved forward with this PR for address #4174 then release notes etc would need to be updated as well. But again, I think it is reasonable to table this for now until #4174 is discussed.
@stefandd if we moved forward with this PR for address #4174 then release notes etc would need to be updated as well. But again, I think it is reasonable to table this for now until #4174 is discussed.
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.
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 main. However, I deleted the offending .md file.
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.
Some happy path tests I would want to see:
- [ ] copy into destination so that it doesn't all fit within the destination where you want to copy something in that is greater in size than destination space
- [ ] copy into the front of destination so copied amount fits within existing destination
- [ ] copy onto the end of a destination so that copied amount fits within the existing destination's space
- [ ] copy into the middle of a destination