Skip to content

Fix conversions of Box<T, A> to Rc<T, A> or Arc<T, A> to use allocator correctly for dropping the Box - #119812

Closed
steffahn wants to merge 1 commit into
rust-lang:masterfrom
steffahn:fix_box_to_rc_conversion
Closed

steffahn wants to merge 1 commit into
rust-lang:masterfrom
steffahn:fix_box_to_rc_conversion

Conversation

@steffahn

Copy link
Copy Markdown
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

rustbot commented Jan 10, 2024

Copy link
Copy Markdown
Collaborator

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

klensy commented Jan 10, 2024

Copy link
Copy Markdown
Contributor

#119801

@steffahn

Copy link
Copy Markdown
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

the8472 commented Jan 10, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member Author

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

@steffahn steffahn closed this Jan 10, 2024
@zachs18

zachs18 commented Jan 10, 2024

Copy link
Copy Markdown
Contributor

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

steffahn commented Jan 10, 2024 •

Copy link
Copy Markdown
Member Author

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