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 chain remaining_mut(), allowing to chain growing buffer #488

Merged
merged 7 commits into from Jun 11, 2022

Conversation

Zettroke
Copy link
Contributor

Stumbled upon panic when was trying to chain fixed buffer with growing buffer.

Fixed remaining_mut for chain, so you can chain growing buffer.

There is workaround for this, so I am not sure if this is intended or not.

let mut buff = [0u8; 128];
let vec = vec![];
let chained = (&mut buff[..]).chain_mut(vec.limit(usize::MAX - buff.len()));

But I decided to fix it, because I think this workaround is inconvenient.

Only potential problem is:
Calls to remaining_mut can return same value (usize::MAX) before and after write to buffer.

let mut buff = [0u8; 128];
let vec = vec![];
let mut chained = (&mut buff[..]).chain_mut(&mut vec);
let prev_remaining = chained.remaining_mut();
chained.put_slice(b"hi");
assert_eq!(prev_remaining, chained.remaining_mut());

@Zettroke Zettroke changed the title Fix chain remaining_mut() allowing to chain growing buffer Fix chain remaining_mut(), allowing to chain growing buffer Feb 27, 2021
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and sorry for the late review!

I tend to favor this, but, I would like to hear others' opinions.

cc @carllerche @seanmonstar @Darksonn: Do you think it's ok to allow remaining_mut to return less value than it actually is? (in the cases it is greater than the usize::MAX, or in all cases?)

tests/test_chain.rs Outdated Show resolved Hide resolved
src/buf/chain.rs Show resolved Hide resolved
@Zettroke
Copy link
Contributor Author

Zettroke commented Aug 7, 2021

Due to #501 I am no longer sure if this PR still useful. To cause overflow now you will need to chain fixed buffer with 2 vectors(or Vec with 2 more), which is very unlikely.

@Darksonn
Copy link
Contributor

Darksonn commented Aug 8, 2021

I think it is fine to allow remaining_mut to be less than the actual capacity.

@taiki-e taiki-e merged commit 3536017 into tokio-rs:master Jun 11, 2022
lelongg pushed a commit to lelongg/bytes that referenced this pull request Jan 9, 2023
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