Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix array and string trim_in_place allocation bounds check #2840

Merged
merged 1 commit into from
Jul 26, 2018

Conversation

dipinhora
Copy link
Contributor

@dipinhora dipinhora commented Jul 26, 2018

Prior to this commit, if Array.trim_in_place and String.trim_in place
trimmed all of the memory allocated for the array or string (i.e.
trim_point == space()) then trim_in_place would still use _ptr._offset
to set the pointer to be the byte after the last byte allocated by the
array or string. This was bad for many reasons but sometimes especially
bad if that next byte happened to be in a different pagemap that had not
yet been allocated. This would result in a segfault the next time reserve
was called because the runtime would think this pointer was not allocated
by pony and complain that it cannot realloc memory not allocated by pony.

This commit fixes the logic in trim_in_place to allocate a new Pointer
if _alloc == 0 (i.e. all of the memory allocated for the array or string
has been trimmed away). This ensures that the _ptr will never point to
memory not owned/allocated by the array also guaranteeing no segfaults
will occur due to unallocated pagemap references.

This commit also modifies the Array.trim_in_place and
String.trim_in_place tests to prevent a regression.

@dipinhora
Copy link
Contributor Author

String.trim_in_place likely needs a similar fix but I haven't looked into it yet.

@mfelsche
Copy link
Contributor

Thanks for this one. The ci failure on aarch64 seems to not give any output for 10 minutes. Most likely a runtime shutdown error, but unrelated.

@SeanTAllen SeanTAllen added triggers release Major issue that when fixed, results in an "emergency" release changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge labels Jul 26, 2018
@SeanTAllen
Copy link
Member

I would restart the failed job but I don't have permissions to do that.

Prior to this commit, if `Array.trim_in_place` and `String.trim_in place`
trimmed all of the memory allocated for the array or string (i.e.
`trim_point == space()`) then trim_in_place would still use _ptr._offset
to set the pointer to be the byte after the last byte allocated by the
array or string. This was bad for many reasons but sometimes especially
bad if that next byte happened to be in a different pagemap that had not
yet been allocated. This would result in a segfault the next time `reserve`
was called because the runtime would think this pointer was not allocated
by pony and complain that it cannot realloc memory not allocated by pony.

This commit fixes the logic in trim_in_place to allocate a new Pointer
if _alloc == 0 (i.e. all of the memory allocated for the array or string
has been trimmed away). This ensures that the `_ptr` will never point to
memory not owned/allocated by the array also guaranteeing no segfaults
will occur due to unallocated pagemap references.

This commit also modifies the `Array.trim_in_place` and
`String.trim_in_place` tests to prevent a regression.
@dipinhora
Copy link
Contributor Author

Grrrrr... Circle CI is very funny when you have your own fork set up to run tests and also open a PR for the same.

I'm almost done with the String.trim_in_place changes and that will automagically cause the tests to get re-run when I push them.

@dipinhora dipinhora changed the title Fix array trim_in_place allocation bounds check Fix array and string trim_in_place allocation bounds check Jul 26, 2018
@dipinhora
Copy link
Contributor Author

The String.trim_in_place changes have been added.

@SeanTAllen SeanTAllen self-requested a review July 26, 2018 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge triggers release Major issue that when fixed, results in an "emergency" release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants