Uh oh!
There was an error while loading. Please reload this page.
std: fix stack buffer overflow in Windows junction_point - #158147
Merged
Conversation
rustbot
commented
Jun 19, 2026
Collaborator
r? @Darksonn rustbot has assigned @Darksonn. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Member
There was a problem hiding this comment.
It sounds like this should be changed to ptr[..abs_path.len()].copy_from_slice(abs_path) or similar so that we actually perform a bounds check here.
Also, it would be really nice with a test that'd catch this (may be easier to check that the test is failing if we insert a bounds check first).
Member
There was a problem hiding this comment.
In fact, maybe the bounds check should be entirely rewritten to
ptr.get(..abs_path.len())
.ok_or_else(|| io::const_error!(io::ErrorKind::InvalidInput, "`original` path is too long"))?
.copy_from_slice(abs_path)
This way there's no risk that the two bounds checks are not kept in sync. For instance, why is there a + 8 in the previous check? I don't understand that part.
Comment on lines
1679
to
1683
| PathBuffer: [MaybeUninit<u16>; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE as usize], | ||
| } | ||
| let data_len = 12 + (abs_path.len() * 2); | ||
| if data_len > u16::MAX as usize { | ||
| if data_len + 8 > c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE as usize { | ||
| return Err(io::const_error!(io::ErrorKind::InvalidInput, "`original` path is too long")); |
Member
There was a problem hiding this comment.
data_len counts the number of bytes, but MAXIMUM_REPARSE_DATA_BUFFER_SIZE counts the number of u16s. This doesn't sound right.
rustbot
commented
Jun 19, 2026
Collaborator
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
devnexen
commented
Jun 19, 2026
ContributorAuthor
@rustbot ready |
| #[test] | ||
| #[cfg(windows)] | ||
| fn junction_point_overlong_path() { |
Member
There was a problem hiding this comment.
I'd like to see this regression test fail without the change. Do you mind opening a second temporary PR containing just the test so that we can run it through CI? We can close it again when we've confirmed the regression test catches the bug.
Comment on lines
+1694
to
+1700
| PathBuffer: [MaybeUninit::uninit(); c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE as usize], | ||
| PathBuffer: [MaybeUninit::uninit(); c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE as usize / 2], | ||
| }; | ||
| // The path is followed by its null terminator and the (empty) print name's | ||
| // null terminator, so the buffer must hold two extra `u16`s. This single | ||
| // bounds check keeps the buffer, the copy, and `data_len` below in sync; if | ||
| // the path doesn't fit, fail rather than overflow the buffer. | ||
| let Some(ptr) = header.PathBuffer.get_mut(..abs_path.len() + 2) else { | ||
| return Err(io::const_error!(io::ErrorKind::InvalidInput, "`original` path is too long")); | ||
| }; | ||
| ptr[..abs_path.len()].write_copy_of_slice(&abs_path); |
Member
There was a problem hiding this comment.
This buffer is uninitialized. Don't you need to set path[abs_path.len()] and path[abs_path.len()+1] to zero?
Member
There was a problem hiding this comment.
The path has an explicit length so it doesn't need to be null terminated at all. I'm uncertain where that idea came from but it's probably just a misunderstanding? I mean, I guess there's no harm in writing a null but if so it shouldn't be part of the length.
ChrisDenton
commented
Jun 19, 2026
Member
To give some context here, this was originally a hacky function used only in tests. It's currently publicly exposed as a nightly-only API but it's not considered ready for stabilisation. E.g. 16kb is way too much for a stack buffer (though admittedly using a stack buffer for shorter paths would be useful). |
| PathBuffer: [MaybeUninit<u16>; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE as usize], | ||
| // `MAXIMUM_REPARSE_DATA_BUFFER_SIZE` is a size in bytes; halve it for a | ||
| // count of `u16`s (the `readlink` path uses it as a byte buffer). | ||
| PathBuffer: [MaybeUninit<u16>; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE as usize / 2], |
Member
There was a problem hiding this comment.
This doesn't sound right at all. What makes you say MAXIMUM_REPARSE_DATA_BUFFER_SIZE is in bytes?
Member
There was a problem hiding this comment.
Huh, apparently it is.
The protocol docs make no mention of a maximum size, only that it's a 16 bit unsigned integer (hence my surprise at 16kb being a limit). But the public headers for the kernel API do have MAXIMUM_REPARSE_DATA_BUFFER_SIZE in bytes of 16kb.
Member
There was a problem hiding this comment.
Should it be changed to an u8 array as well?
ContributorAuthor
There was a problem hiding this comment.
I would say no. It's the mount-point WCHAR path, and u16 keeps the copy a clean write_copy_of_slice(&abs_path).
devnexenforce-pushed
the
windows_fs_oflow_fix
branch
from
June 21, 2026 06:05
d842451 to
28748e5Comparerust-borsBot
pushed a commit
that referenced
this pull request
Jun 21, 2026
Temporary CI-verification test for #158147. Without the fix, a >16 KiB junction target passes the old `> u16::MAX` length check yet overflows the inline reparse stack buffer. This test must fail on master and pass once the fix lands.
devnexen
commented
Jun 21, 2026
ContributorAuthor
@rustbot ready |
| // Size of the reparse data after the 8-byte ReparseTag/ReparseDataLength/ | ||
| // Reserved header: the four name offset/length `u16` fields (8 bytes) plus | ||
| // the path. | ||
| let data_len = 8 + abs_path.len() * 2; |
Member
There was a problem hiding this comment.
Isn't it 16 bytes?
- ReparseTag is 4 bytes
- ReparseDataLength is 2 bytes
- Reserved is 2 bytes
- SubstituteNameOffset is 2 bytes
- SubstituteNameLength is 2 bytes
- PrintNameOffset is 2 bytes
- PrintNameLength is 2 bytes
ContributorAuthor
There was a problem hiding this comment.
Right, it's 16 bytes. Switched to offset_of! so both lengths derive from the struct.
devnexenforce-pushed
the
windows_fs_oflow_fix
branch
from
June 22, 2026 23:24
28748e5 to
b7af0d1Comparedevnexen
commented
Jun 23, 2026
ContributorAuthor
@rustbot ready |
Contributor
This pull request was unapproved. |
Darksonn
commented
Jul 25, 2026
Member
@devnexen Any update on this? |
devnexenforce-pushed
the
windows_fs_oflow_fix
branch
from
July 25, 2026 15:16
b7af0d1 to
74afb0aCompare
This comment has been minimized.
This comment has been minimized.
The guard checked `data_len > u16::MAX`, allowing paths far larger than `PathBuffer` (a fixed 16384-element array), which the subsequent single `copy_from` then overflows. Bound against `MAXIMUM_REPARSE_DATA_BUFFER_SIZE` plus header instead, matching the kernel's limit.
devnexenforce-pushed
the
windows_fs_oflow_fix
branch
from
July 25, 2026 15:32
74afb0a to
3313cd6Comparerustbot
commented
Jul 25, 2026
Collaborator
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Darksonn
commented
Aug 5, 2026
Member
Please remember to use @bors try |
This comment has been minimized.
This comment has been minimized.
rust-borsBot
pushed a commit
that referenced
this pull request
Aug 5, 2026
std: fix stack buffer overflow in Windows junction_point
Contributor
Contributor
JonathanBrouwer added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Aug 5, 2026
…arksonn std: fix stack buffer overflow in Windows junction_point The guard checked `data_len > u16::MAX`, allowing paths far larger than `PathBuffer` (a fixed 16384-element array), which the subsequent single `copy_from` then overflows. Bound against `MAXIMUM_REPARSE_DATA_BUFFER_SIZE` plus header instead, matching the kernel's limit.
rust-borsBot
pushed a commit
that referenced
this pull request
Aug 5, 2026
…uwer Rollup of 22 pull requests Successful merges: - #158147 (std: fix stack buffer overflow in Windows junction_point) - #159784 (Hint that memchr returns an in-bounds index) - #160130 (Select cache values to verify by key fingerprint, not value fingerprint) - #160343 (Rename `OutlivesPredicate` to `OutlivesClause`) - #160360 (Remove rustc_middle dependency on rustc_hir_pretty) - #160387 (rustc_codegen_ssa: Correctly apply the static `--jobs-backend` limit to backend parallelism) - #160422 (move mir-opt miri tests to CI logic) - #160444 (Avoid resolving path keywords outside `TypeNS`) - #160510 (Resolver: (un)tracked borrows for `CmRefCell` made safe my unsafe speculative flag) - #155424 ([blocked] Link to proposed LLM policy in CONTRIBUTING and pull request template) - #158726 (std: move futex implementations into sys::sync::futex) - #159225 (Split IncrCompSession out of Session) - #159820 (Make the `rustc_unsafe_specialization_marker` attribute actually `unsafe`) - #160198 (Rework `smallest_range_containing` to handle duplicates) - #160362 (Split `SpannedTypeVisitor` into its own crate, `rustc_ty_walk`) - #160390 (autodiff: Handle slice-tailed DSTs in type trees) - #160420 (Suggest `cast_signed()` for overflowing signed integer literals) - #160520 (Add some tests for specialization) - #160522 (fix(bootstrap): Normalize the names of proc macro dependency crates) - #160531 (docs: fix typo in AllowExprMetavar comment) - #160538 (Update expect messages in tcp.rs doc examples to follow the style guide) - #160548 (bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step)
JonathanBrouwer added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Aug 5, 2026
…arksonn std: fix stack buffer overflow in Windows junction_point The guard checked `data_len > u16::MAX`, allowing paths far larger than `PathBuffer` (a fixed 16384-element array), which the subsequent single `copy_from` then overflows. Bound against `MAXIMUM_REPARSE_DATA_BUFFER_SIZE` plus header instead, matching the kernel's limit.
rust-borsBot
pushed a commit
that referenced
this pull request
Aug 5, 2026
…uwer Rollup of 25 pull requests Successful merges: - #154585 (treat no_mangle_generic_items as hard error instead of lint warning) - #158147 (std: fix stack buffer overflow in Windows junction_point) - #160130 (Select cache values to verify by key fingerprint, not value fingerprint) - #160343 (Rename `OutlivesPredicate` to `OutlivesClause`) - #160360 (Remove rustc_middle dependency on rustc_hir_pretty) - #160387 (rustc_codegen_ssa: Correctly apply the static `--jobs-backend` limit to backend parallelism) - #160422 (move mir-opt miri tests to CI logic) - #160444 (Avoid resolving path keywords outside `TypeNS`) - #160510 (Resolver: (un)tracked borrows for `CmRefCell` made safe my unsafe speculative flag) - #155424 ([blocked] Link to proposed LLM policy in CONTRIBUTING and pull request template) - #158726 (std: move futex implementations into sys::sync::futex) - #158762 (Emit thumb code on VEX V5) - #159225 (Split IncrCompSession out of Session) - #159820 (Make the `rustc_unsafe_specialization_marker` attribute actually `unsafe`) - #160198 (Rework `smallest_range_containing` to handle duplicates) - #160362 (Split `SpannedTypeVisitor` into its own crate, `rustc_ty_walk`) - #160390 (autodiff: Handle slice-tailed DSTs in type trees) - #160420 (Suggest `cast_signed()` for overflowing signed integer literals) - #160516 (Add regression test for HRTB projection in closure) - #160520 (Add some tests for specialization) - #160522 (fix(bootstrap): Normalize the names of proc macro dependency crates) - #160523 (Add regression test for opaque type) - #160531 (docs: fix typo in AllowExprMetavar comment) - #160538 (Update expect messages in tcp.rs doc examples to follow the style guide) - #160548 (bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step)
rust-borsBot
pushed a commit
that referenced
this pull request
Aug 5, 2026
…uwer Rollup of 25 pull requests Successful merges: - #158147 (std: fix stack buffer overflow in Windows junction_point) - #160130 (Select cache values to verify by key fingerprint, not value fingerprint) - #160343 (Rename `OutlivesPredicate` to `OutlivesClause`) - #160360 (Remove rustc_middle dependency on rustc_hir_pretty) - #160387 (rustc_codegen_ssa: Correctly apply the static `--jobs-backend` limit to backend parallelism) - #160422 (move mir-opt miri tests to CI logic) - #160444 (Avoid resolving path keywords outside `TypeNS`) - #160510 (Resolver: (un)tracked borrows for `CmRefCell` made safe by unsafe speculative flag) - #155424 ([blocked] Link to proposed LLM policy in CONTRIBUTING and pull request template) - #158726 (std: move futex implementations into sys::sync::futex) - #158762 (Emit thumb code on VEX V5) - #159225 (Split IncrCompSession out of Session) - #159820 (Make the `rustc_unsafe_specialization_marker` attribute actually `unsafe`) - #160198 (Rework `smallest_range_containing` to handle duplicates) - #160362 (Split `SpannedTypeVisitor` into its own crate, `rustc_ty_walk`) - #160390 (autodiff: Handle slice-tailed DSTs in type trees) - #160420 (Suggest `cast_signed()` for overflowing signed integer literals) - #160501 (Add bootstrap CLI snapshot test for testing miri) - #160516 (Add regression test for HRTB projection in closure) - #160520 (Add some tests for specialization) - #160522 (fix(bootstrap): Normalize the names of proc macro dependency crates) - #160523 (Add regression test for opaque type) - #160531 (docs: fix typo in AllowExprMetavar comment) - #160538 (Update expect messages in tcp.rs doc examples to follow the style guide) - #160548 (bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step)
Uh oh!
There was an error while loading. Please reload this page.
rust-timer added a commit
that referenced
this pull request
Aug 5, 2026
Rollup merge of #158147 - devnexen:windows_fs_oflow_fix, r=Darksonn std: fix stack buffer overflow in Windows junction_point The guard checked `data_len > u16::MAX`, allowing paths far larger than `PathBuffer` (a fixed 16384-element array), which the subsequent single `copy_from` then overflows. Bound against `MAXIMUM_REPARSE_DATA_BUFFER_SIZE` plus header instead, matching the kernel's limit.
github-actionsBot
pushed a commit
to rust-lang/rustc-dev-guide
that referenced
this pull request
Aug 10, 2026
…uwer Rollup of 25 pull requests Successful merges: - rust-lang/rust#158147 (std: fix stack buffer overflow in Windows junction_point) - rust-lang/rust#160130 (Select cache values to verify by key fingerprint, not value fingerprint) - rust-lang/rust#160343 (Rename `OutlivesPredicate` to `OutlivesClause`) - rust-lang/rust#160360 (Remove rustc_middle dependency on rustc_hir_pretty) - rust-lang/rust#160387 (rustc_codegen_ssa: Correctly apply the static `--jobs-backend` limit to backend parallelism) - rust-lang/rust#160422 (move mir-opt miri tests to CI logic) - rust-lang/rust#160444 (Avoid resolving path keywords outside `TypeNS`) - rust-lang/rust#160510 (Resolver: (un)tracked borrows for `CmRefCell` made safe by unsafe speculative flag) - rust-lang/rust#155424 ([blocked] Link to proposed LLM policy in CONTRIBUTING and pull request template) - rust-lang/rust#158726 (std: move futex implementations into sys::sync::futex) - rust-lang/rust#158762 (Emit thumb code on VEX V5) - rust-lang/rust#159225 (Split IncrCompSession out of Session) - rust-lang/rust#159820 (Make the `rustc_unsafe_specialization_marker` attribute actually `unsafe`) - rust-lang/rust#160198 (Rework `smallest_range_containing` to handle duplicates) - rust-lang/rust#160362 (Split `SpannedTypeVisitor` into its own crate, `rustc_ty_walk`) - rust-lang/rust#160390 (autodiff: Handle slice-tailed DSTs in type trees) - rust-lang/rust#160420 (Suggest `cast_signed()` for overflowing signed integer literals) - rust-lang/rust#160501 (Add bootstrap CLI snapshot test for testing miri) - rust-lang/rust#160516 (Add regression test for HRTB projection in closure) - rust-lang/rust#160520 (Add some tests for specialization) - rust-lang/rust#160522 (fix(bootstrap): Normalize the names of proc macro dependency crates) - rust-lang/rust#160523 (Add regression test for opaque type) - rust-lang/rust#160531 (docs: fix typo in AllowExprMetavar comment) - rust-lang/rust#160538 (Update expect messages in tcp.rs doc examples to follow the style guide) - rust-lang/rust#160548 (bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step)
flip1995 pushed a commit
to flip1995/rust-clippy
that referenced
this pull request
Aug 17, 2026
…uwer Rollup of 25 pull requests Successful merges: - rust-lang/rust#158147 (std: fix stack buffer overflow in Windows junction_point) - rust-lang/rust#160130 (Select cache values to verify by key fingerprint, not value fingerprint) - rust-lang/rust#160343 (Rename `OutlivesPredicate` to `OutlivesClause`) - rust-lang/rust#160360 (Remove rustc_middle dependency on rustc_hir_pretty) - rust-lang/rust#160387 (rustc_codegen_ssa: Correctly apply the static `--jobs-backend` limit to backend parallelism) - rust-lang/rust#160422 (move mir-opt miri tests to CI logic) - rust-lang/rust#160444 (Avoid resolving path keywords outside `TypeNS`) - rust-lang/rust#160510 (Resolver: (un)tracked borrows for `CmRefCell` made safe by unsafe speculative flag) - rust-lang/rust#155424 ([blocked] Link to proposed LLM policy in CONTRIBUTING and pull request template) - rust-lang/rust#158726 (std: move futex implementations into sys::sync::futex) - rust-lang/rust#158762 (Emit thumb code on VEX V5) - rust-lang/rust#159225 (Split IncrCompSession out of Session) - rust-lang/rust#159820 (Make the `rustc_unsafe_specialization_marker` attribute actually `unsafe`) - rust-lang/rust#160198 (Rework `smallest_range_containing` to handle duplicates) - rust-lang/rust#160362 (Split `SpannedTypeVisitor` into its own crate, `rustc_ty_walk`) - rust-lang/rust#160390 (autodiff: Handle slice-tailed DSTs in type trees) - rust-lang/rust#160420 (Suggest `cast_signed()` for overflowing signed integer literals) - rust-lang/rust#160501 (Add bootstrap CLI snapshot test for testing miri) - rust-lang/rust#160516 (Add regression test for HRTB projection in closure) - rust-lang/rust#160520 (Add some tests for specialization) - rust-lang/rust#160522 (fix(bootstrap): Normalize the names of proc macro dependency crates) - rust-lang/rust#160523 (Add regression test for opaque type) - rust-lang/rust#160531 (docs: fix typo in AllowExprMetavar comment) - rust-lang/rust#160538 (Update expect messages in tcp.rs doc examples to follow the style guide) - rust-lang/rust#160548 (bootstrap: Register `coverage-map` and `coverage-run` aliases via a separate step)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
The guard checked
data_len > u16::MAX, allowing paths far larger thanPathBuffer(a fixed 16384-element array), which the subsequent singlecopy_fromthen overflows. Bound againstMAXIMUM_REPARSE_DATA_BUFFER_SIZEplus header instead, matching the kernel's limit.