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 memory corruption with Array.chop and String.chop #3755

Merged
merged 1 commit into from
May 6, 2021
Merged

Conversation

SeanTAllen
Copy link
Member

Both Array.chop and String.chop violate an expectation that the memory
allocator had.

The memory allocator was written before the standard library and assumed
it would be responsible for doing "smart things" with realloc. That is,
when you ask for a realloc, perhaps you don't really need it. Perhaps
you already have enough.

This worked without issue in the standard library until chop came along.
Chop turns a single previous object allocation into 2 and that confused
the memory allocator and could lead to it not reallocating (malloc and copy)
when safety required it. Instead it would return the same pointer it was given.

Based on how the Pony standard library ended up, with String and Array understanding
how much they allocated, we do not need that realloc behavior.

This commit removes the behavior from realloc and leaves it so that it will allows
do a malloc and copy which is what the standard library code expects it to do.

Both Array.chop and String.chop violate an expectation that the memory
allocator had.

The memory allocator was written before the standard library and assumed
it would be responsible for doing "smart things" with realloc. That is,
when you ask for a realloc, perhaps you don't really need it. Perhaps
you already have enough.

This worked without issue in the standard library until `chop` came along.
Chop turns a single previous object allocation into 2 and that confused
the memory allocator and could lead to it not reallocating (malloc and copy)
when safety required it. Instead it would return the same pointer it was given.

Based on how the Pony standard library ended up, with String and Array understanding
how much they allocated, we do not need that realloc behavior.

This commit removes the behavior from realloc and leaves it so that it will allows
do a malloc and copy which is what the standard library code expects it to do.
@SeanTAllen SeanTAllen requested a review from jemc May 6, 2021 15:32
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label May 6, 2021
@SeanTAllen
Copy link
Member Author

@jemc if this is good to you, once it passes CI, if you are around, please merge, otherwise I will get later. I'd like to get in prior to 3:50 PM pacific so it makes it into tonight's nightly.

@SeanTAllen SeanTAllen merged commit dae95a0 into main May 6, 2021
@SeanTAllen SeanTAllen deleted the realloc-fix branch May 6, 2021 16:21
github-actions bot pushed a commit that referenced this pull request May 6, 2021
github-actions bot pushed a commit that referenced this pull request May 6, 2021
jemc added a commit to savi-lang/savi that referenced this pull request May 16, 2021
Note that this required importing a patch from a recent PR in ponyc:
ponylang/ponyc#3755

This PR was the result of me pointing out a memory safety issue with chop in ponyc,
due to a violation of some outdated assumptions in libponyrt's reallocaion logic.
See this thread in Zulip for more details:
https://ponylang.zulipchat.com/#narrow/stream/189934-general/topic/String.2Echop.20and.20Array.2Echop.20safety.20issue.20.28EDIT.3A.20bug.20has.20be.2E.2E.2E/near/237578974
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants