Skip to content

Memory leak in owned_to_vec #773

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

Merged
merged 4 commits into from
Mar 5, 2025
Merged

Conversation

venomousmoog
Copy link
Contributor

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.

Copy link
Contributor

@Darksonn 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.

@Darksonn Darksonn merged commit 3667543 into tokio-rs:master Mar 5, 2025
18 checks passed
@scottlamb
Copy link
Contributor

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.

@Darksonn
Copy link
Contributor

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.

@scottlamb
Copy link
Contributor

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants