Skip to content

Conversation

conradludgate
Copy link
Contributor

Fixes #120603 by checking the returned read n is in-bounds of the cursor.

Interestingly, I noticed that BorrowedBuf side-steps this issue by using checked accesses. Maybe this can be switched to unchecked to mirror what BufReader does

unsafe { MaybeUninit::slice_assume_init_ref(&self.buf[0..self.filled]) }

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2024

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@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 Feb 3, 2024
@conradludgate
Copy link
Contributor Author

While fixing this, I noticed that Take has a similar panic, which reassures me that panicking here is the correct (or at least consistent) thing to do:

assert!(n as u64 <= self.limit, "number of read bytes exceeds limit");

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Feb 3, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 3, 2024

📌 Commit 4c694db has been approved by dtolnay

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 3, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Feb 3, 2024
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se Feb 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
fix rust-lang#120603 by adding a check in default_read_buf

Fixes rust-lang#120603 by checking the returned read n is in-bounds of the cursor.

Interestingly, I noticed that `BorrowedBuf` side-steps this issue by using checked accesses. Maybe this can be switched to unchecked to mirror what BufReader does https://github.com/rust-lang/rust/blob/bf3c6c5bed498f41ad815641319a1ad9bcecb8e8/library/core/src/io/borrowed_buf.rs#L95
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 4, 2024
fix rust-lang#120603 by adding a check in default_read_buf

Fixes rust-lang#120603 by checking the returned read n is in-bounds of the cursor.

Interestingly, I noticed that `BorrowedBuf` side-steps this issue by using checked accesses. Maybe this can be switched to unchecked to mirror what BufReader does https://github.com/rust-lang/rust/blob/bf3c6c5bed498f41ad815641319a1ad9bcecb8e8/library/core/src/io/borrowed_buf.rs#L95
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120518 (riscv only supports split_debuginfo=off for now)
 - rust-lang#120521 (Make `NonZero` constructors generic.)
 - rust-lang#120527 (Switch OwnedStore handle count to AtomicU32)
 - rust-lang#120550 (Continue to borrowck even if there were previous errors)
 - rust-lang#120587 (miri: normalize struct tail in ABI compat check)
 - rust-lang#120590 (Remove unused args from functions)
 - rust-lang#120607 (fix rust-lang#120603 by adding a check in default_read_buf)

Failed merges:

 - rust-lang#120575 (Simplify codegen diagnostic handling)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119481 (Clarify ambiguity in select_nth_unstable docs)
 - rust-lang#119600 (Remove outdated references to librustc_middle)
 - rust-lang#120458 (Document `&CStr` to `CString` conversion)
 - rust-lang#120569 (coverage: Improve handling of function/closure spans)
 - rust-lang#120572 (Update libc to 0.2.153)
 - rust-lang#120587 (miri: normalize struct tail in ABI compat check)
 - rust-lang#120607 (fix rust-lang#120603 by adding a check in default_read_buf)
 - rust-lang#120636 (Subtree update of `rust-analyzer`)
 - rust-lang#120641 (rustdoc: trait.impl, type.impl: sort impls to make it not depend on serialization order)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d8e9ddc into rust-lang:master Feb 5, 2024
@rustbot rustbot added this to the 1.78.0 milestone Feb 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
Rollup merge of rust-lang#120607 - conradludgate:fix-120603, r=dtolnay

fix rust-lang#120603 by adding a check in default_read_buf

Fixes rust-lang#120603 by checking the returned read n is in-bounds of the cursor.

Interestingly, I noticed that `BorrowedBuf` side-steps this issue by using checked accesses. Maybe this can be switched to unchecked to mirror what BufReader does https://github.com/rust-lang/rust/blob/bf3c6c5bed498f41ad815641319a1ad9bcecb8e8/library/core/src/io/borrowed_buf.rs#L95
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.

Unsoundness in BufReader with a broken inner Read impl
5 participants