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 conversions of Box<T, A> to Rc<T, A> or Arc<T, A> to use allocator correctly for dropping the Box #119812

Closed
wants to merge 1 commit into from

Conversation

steffahn
Copy link
Member

This fixes #119749.

I wanted to add a test, too, but realized there are essentially no tests for almost any of the code handling custom allocators, anyways1, and I didn’t want to introduce a whole generally usable testing Allocator implementation just for this soundness fix.

Footnotes

  1. as far as I could tell. Feel free to point me to anything I might have missed.

to use allocator correctly for dropping the `Box`.
@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2024

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 10, 2024
@klensy
Copy link
Contributor

klensy commented Jan 10, 2024

#119801

@steffahn
Copy link
Member Author

Looks like the issue was independently discovered there too, perhaps, otherwise #119749 would have been linked?

I didn't know that miri does differentiate Global from System, that may make adding tests easier - though.. are (all or some) library tests run through miri on CI?

@the8472
Copy link
Member

the8472 commented Jan 10, 2024

CI doesn't run lib tests under miri, that happens somewhere else as a daily job because it's slow. You can do that manually via https://github.com/rust-lang/miri-test-libstd
In CI miri only runs some basic tests from src/tools/miri/tests

@steffahn
Copy link
Member Author

I meant CI more generally anyways - so I guess a normal library test would be sufficient to make sure a bug being reintroduced wouldn't stay unnoticed. Thanks for the info, good to know.

@steffahn
Copy link
Member Author

Anyways… I guess I’ll close this to remove the duplicate :-)

@steffahn steffahn closed this Jan 10, 2024
@zachs18
Copy link
Contributor

zachs18 commented Jan 10, 2024

Ah sorry, I didn't see there was already an issue for it. I discovered the problem while working on #119761 but wanted to fix it in a separate PR since it was a soundness fix which shouldn't need as much approval as that PR.

@steffahn
Copy link
Member Author

steffahn commented Jan 10, 2024

No worries. We had literally seeing the same issue independently around roughly the same time. Great coincidence. I’ve spotted your PR #119761 already after #119801 was mentioned, and figured that’s where you’re coming from. While you were creating #119761, the issue #119749 probably didn't even exist yet. I was coming from explaining ppl how Rc can support unsized contents, and pointed out the Box<T> -> Rc<T> conversion as an example how to move around unsized data; in the process of that noticing the bug in the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std's From<Box<T, A>> for Rc<T, A> implementation is unsound when A is a custom allocator.
6 participants