Skip to content

Conversation

cammeresi
Copy link
Contributor

@cammeresi cammeresi commented Sep 21, 2025

Memory was allocated via Box::leak and thence intended to be tracked and deallocated manually, but the allocator was also leaked, not tracked, and never dropped. Now it is dropped immediately.

According to my reading of the Allocator trait, if a copy of the allocator remains live, then its allocations must remain live. Since the B-tree has a copy of the allocator that will only be dropped after the nodes, it's safe to not store the allocator in each node (which would be a much more intrusive change).

Fixes: #106203

@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 Sep 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 21, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct, but could you add a comment explaining the allocator behaviour you outlined in the PR description?

View changes since this review

@joboet joboet assigned joboet and unassigned ibraheemdev Sep 24, 2025
Memory was allocated via `Box::leak` and thence intended to be tracked
and deallocated manually, but the allocator was also leaked, not
tracked, and never dropped.  Now it is dropped immediately.

According to my reading of the `Allocator` trait, if a copy of the
allocator remains live, then its allocations must remain live.  Since
the B-tree has a copy of the allocator that will only be dropped after
the nodes, it's safe to not store the allocator in each node (which
would be a much more intrusive change).
@joboet
Copy link
Member

joboet commented Sep 25, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 25, 2025

📌 Commit 137d4ea has been approved by joboet

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 25, 2025
…joboet

BTreeMap: Don't leak allocators when initializing nodes

Memory was allocated via `Box::leak` and thence intended to be tracked and deallocated manually, but the allocator was also leaked, not tracked, and never dropped.  Now it is dropped immediately.

According to my reading of the `Allocator` trait, if a copy of the allocator remains live, then its allocations must remain live.  Since the B-tree has a copy of the allocator that will only be dropped after the nodes, it's safe to not store the allocator in each node (which would be a much more intrusive change).

Fixes: rust-lang#106203
bors added a commit that referenced this pull request Sep 25, 2025
Rollup of 8 pull requests

Successful merges:

 - #116882 (rustdoc: hide `#[repr]` if it isn't part of the public ABI)
 - #135771 ([rustdoc] Add support for associated items in "jump to def" feature)
 - #141032 (avoid violating `slice::from_raw_parts` safety contract in `Vec::extract_if`)
 - #142401 (Add proper name mangling for pattern types)
 - #146293 (feat: non-panicking `Vec::try_remove`)
 - #146859 (BTreeMap: Don't leak allocators when initializing nodes)
 - #146924 (Add doc for `NonZero*` const creation)
 - #146933 (Make `render_example_with_highlighting` return an `impl fmt::Display`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 83cf8f9 into rust-lang:master Sep 25, 2025
10 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 25, 2025
rust-timer added a commit that referenced this pull request Sep 25, 2025
Rollup merge of #146859 - cammeresi:btree-alloc-20250920, r=joboet

BTreeMap: Don't leak allocators when initializing nodes

Memory was allocated via `Box::leak` and thence intended to be tracked and deallocated manually, but the allocator was also leaked, not tracked, and never dropped.  Now it is dropped immediately.

According to my reading of the `Allocator` trait, if a copy of the allocator remains live, then its allocations must remain live.  Since the B-tree has a copy of the allocator that will only be dropped after the nodes, it's safe to not store the allocator in each node (which would be a much more intrusive change).

Fixes: #106203
@cammeresi cammeresi deleted the btree-alloc-20250920 branch September 26, 2025 00:02
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 26, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang/rust#116882 (rustdoc: hide `#[repr]` if it isn't part of the public ABI)
 - rust-lang/rust#135771 ([rustdoc] Add support for associated items in "jump to def" feature)
 - rust-lang/rust#141032 (avoid violating `slice::from_raw_parts` safety contract in `Vec::extract_if`)
 - rust-lang/rust#142401 (Add proper name mangling for pattern types)
 - rust-lang/rust#146293 (feat: non-panicking `Vec::try_remove`)
 - rust-lang/rust#146859 (BTreeMap: Don't leak allocators when initializing nodes)
 - rust-lang/rust#146924 (Add doc for `NonZero*` const creation)
 - rust-lang/rust#146933 (Make `render_example_with_highlighting` return an `impl fmt::Display`)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Memory leaks in BTreeMap with allocator_api enabled
5 participants