Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

API soundness issue in join() implementation of [Borrow<str>] #80335

Closed
Qwaz opened this issue Dec 23, 2020 · 2 comments · Fixed by #81728
Closed

API soundness issue in join() implementation of [Borrow<str>] #80335

Qwaz opened this issue Dec 23, 2020 · 2 comments · Fixed by #81728
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Qwaz
Copy link
Contributor

Qwaz commented Dec 23, 2020

A weird Borrow implementation that returns a different result for each call can create a string with uninitialized bytes with join() implementation of [Borrow<str>] type.

The problem is in join_generic_copy function.

  1. The borrow result is first used for the length calculation.

    // compute the exact total length of the joined Vec
    // if the `len` calculation overflows, we'll panic
    // we would have run out of memory anyway and the rest of the function requires
    // the entire Vec pre-allocated for safety
    let len = sep_len
    .checked_mul(iter.len())
    .and_then(|n| {
    slice.iter().map(|s| s.borrow().as_ref().len()).try_fold(n, usize::checked_add)
    })
    .expect("attempt to join into collection with len > usize::MAX");

  2. Then, inside spezialize_for_lengths macro, the user-provided slice is borrowed again and the content is copied.

    // arbitrary non-zero size fallback
    for s in iter {
    copy_slice_and_advance!(target, sep_bytes);
    copy_slice_and_advance!(target, s.borrow().as_ref());
    }

  3. Finally, the length of the slice is set to the length calculated in step 1.

    result.set_len(len);

Playground link, which demonstrates creating a non-UTF-8 string by only using safe Rust.

@Qwaz Qwaz added the C-bug Category: This is a bug. label Dec 23, 2020
@Mark-Simulacrum Mark-Simulacrum added I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 23, 2020
@LeSeulArtichaut LeSeulArtichaut added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 23, 2020
@LeSeulArtichaut
Copy link
Contributor

@steffahn
Copy link
Member

I guess all that’s needed is an assert!(target.is_empty()) at the end of the body of the spezialize_for_lengths macro? Perhaps the typo in its name could be fixed, too. It might also be beneficial to avoid the unnecessary third(!) call to borrow() that’s due to copy_slice_and_advance using its second argument twice.

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 28, 2021
@bors bors closed this as completed in 6d43225 Mar 28, 2021
cuviper pushed a commit to cuviper/rust that referenced this issue Apr 26, 2021
(cherry picked from commit 6d43225)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants