Light Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Memory leak in owned_to_vec#773

Merged
Darksonn merged 4 commits intotokio-rs:masterfrom
venomousmoog:owned_vec_into
Mar 5, 2025
Merged

Memory leak in owned_to_vec#773
Darksonn merged 4 commits intotokio-rs:masterfrom
venomousmoog:owned_vec_into

Conversation

Copy link
Contributor

venomousmoog commented Mar 4, 2025

The following sequence:

let b1 = Bytes::from_owner(owner);
let v1: Vec<u8> = b1.into();

Would fail to drop owner (the expectation for the vtable *_to_vec is that it takes ownership of the instance, but owned_to_vec was not). Since owned_to_mut depended on owned_to_vec and was correctly dropping, the fix was to move the drop logic from owned_to_mut to owned_to_vec.

This change adds an appropriate test for ::into() for Bytes::from_owner.

Darksonn approved these changes Mar 5, 2025
Copy link
Member

Darksonn left a comment

Choose a reason for hiding this comment

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

Thank you.

venomousmoog reacted with thumbs up emoji
Darksonn merged commit 3667543 into tokio-rs:master Mar 5, 2025
18 checks passed
Darksonn mentioned this pull request Mar 5, 2025
Copy link
Contributor

scottlamb commented Mar 5, 2025

the expectation for the vtable *_to_vec is that it takes ownership of the instance, but owned_to_vec was not

Should the to_vec and to_mut vtable entries be called into_vec and into_mut, respectively, to make this misunderstanding less likely?

I also wonder if the owned_to_vec test in test_bytes.rs is accomplishing anything, given that it's using slice::to_vec rather than a Bytes method. I suspect it was meant to exercise the behavior the new owner_into_vec test does.

Copy link
Member

Darksonn commented Mar 6, 2025

Yes, that's an excellent idea. Do you want to submit a PR to rename them?

I did run the test locally before merging this, and the test failed without this fix.

Copy link
Contributor

scottlamb commented Mar 6, 2025

Yes, that's an excellent idea. Do you want to submit a PR to rename them?

Will do.

I did run the test locally before merging this, and the test failed without this fix.

The new owned_into_vec test, right? The pfd-existing owned_to_vec above it is the one I'm referring to.

scottlamb added a commit to scottlamb/bytes that referenced this pull request Mar 6, 2025
The new name matches the standard convention for methods that
consume `self`, as referred to in the following clippy lint:
<https://rust-lang.github.io/rust-clippy/stable/index.html#wrong_self_convention>

The hope is by following convention we avoid bugs such as that fixed
in tokio-rs#773.
scottlamb added a commit to scottlamb/bytes that referenced this pull request Mar 6, 2025
The new name matches the Rust convention for methods that
consume `self`, as referred to in the following clippy lint:
<https://rust-lang.github.io/rust-clippy/stable/index.html#wrong_self_convention>
This may help avoid bugs such as that fixed in tokio-rs#773.
scottlamb mentioned this pull request Mar 6, 2025
mattwildermuth pushed a commit to mattwildermuth/external_rust_android-crates-io that referenced this pull request Nov 2, 2025
This release contains a fix for a memory leak.

The patch to tests/test_bytes.rs is no longer necessary because the
change has been sent upstream.

Link: tokio-rs/bytes#773
Change-Id: I9d0d6289769fb07b378ad030d269d3edbe52b7b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Darksonn Darksonn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants