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

Tracking Issue for junction_point #121709

Open
3 tasks
ChrisDenton opened this issue Feb 27, 2024 · 4 comments
Open
3 tasks

Tracking Issue for junction_point #121709

ChrisDenton opened this issue Feb 27, 2024 · 4 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ChrisDenton
Copy link
Member

ChrisDenton commented Feb 27, 2024

Feature gate: #![feature(junction_point)]

This is a tracking issue for rust-lang/libs-team#203

On Windows, it allows creating a juncture point. Which is useful if/when symlinks are not enabled.

Public API

// std::os::windows::fs::junction_point
fn junction_point<P: AsRef<Path>, Q: AsRef<Path>>(
    original: P,
    link: Q
) -> Result<()>;

Steps / History

Unresolved Questions

  • None yet.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@ChrisDenton ChrisDenton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Feb 27, 2024
Nadrieril added a commit to Nadrieril/rust that referenced this issue Mar 9, 2024
…acrum

Implement junction_point

Implements rust-lang#121709

We already had a private implementation that we use for tests so we could just make that public. Except it was very hacky as it was only ever intended for use in testing. I've made an improved version that at least handles path conversion correctly and has less need for things like the `Align8` hack. There's still room for further improvement though.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 9, 2024
Rollup merge of rust-lang#121711 - ChrisDenton:junction, r=Mark-Simulacrum

Implement junction_point

Implements rust-lang#121709

We already had a private implementation that we use for tests so we could just make that public. Except it was very hacky as it was only ever intended for use in testing. I've made an improved version that at least handles path conversion correctly and has less need for things like the `Align8` hack. There's still room for further improvement though.
@sjohannes
Copy link

Some concerns/questions about the current implementation:

  • header should probably be renamed as it's not just the header.

  • My understanding is that, within PathBuffer, abs_path is intended to be followed by 4 bytes of null. But is that currently the case?

  • If the open succeeds but the ioctl fails, does this leave a directory behind?

  • The lpBytesReturned (second last) argument of DeviceIoControl can be null since we're not using it.

@ChrisDenton
Copy link
Member Author

The current implementation was adapted from an internal-only function that was used for testing filesystem behaviour on junction points. Doubtless it can be improved further, PRs welcome!

header should probably be renamed as it's not just the header.

Sure.

My understanding is that, within PathBuffer, abs_path is intended to be followed by 4 bytes of null. But is that currently the case?

No, it actually doesn't need to be followed by any nulls seeing as the length is given in another field. Any implementation that reads the paths and assumes null termination is wrong. We could null terminate as a courtesy to "wrong" code, though such code will be broken on other input. I don't see any need to have a double null. The buffer is not like a flexible array, it can only ever contain exactly two strings which are pointed to by the offset fields.

If the open succeeds but the ioctl fails, does this leave a directory behind?

Yes.

The lpBytesReturned (second last) argument of DeviceIoControl can be null since we're not using it.

From the DeviceIoControl documentation:

If lpOverlapped is NULL, lpBytesReturned cannot be NULL. Even when an operation returns no output data and lpOutBuffer is NULL, DeviceIoControl makes use of lpBytesReturned. After such an operation, the value of lpBytesReturned is meaningless.

@sjohannes
Copy link

No, it actually doesn't need to be followed by any nulls seeing as the length is given in another field. Any implementation that reads the paths and assumes null termination is wrong. We could null terminate as a courtesy to "wrong" code, though such code will be broken on other input. I don't see any need to have a double null. The buffer is not like a flexible array, it can only ever contain exactly two strings which are pointed to by the offset fields.

That is correct; however, if I'm counting things right, data_len there adds four extra bytes (12 = 8 (MountPointReparseBuffer headers) + 4 (nulls)), which makes me think that the intention was to add those nulls to match some other implementation. Also note that PrintNameOffset has a +1, again looking like it was intended to accommodate the null.

By the way, just as an interesting observation, PowerShell New-Item uses an empty string for PrintName (like what we do), whereas cmd.exe mklink duplicates the absolute path (without \??\) there. I haven't been able to find out what exactly PrintName is supposed to be for.

From the DeviceIoControl documentation:

If lpOverlapped is NULL, lpBytesReturned cannot be NULL. Even when an operation returns no output data and lpOutBuffer is NULL, DeviceIoControl makes use of lpBytesReturned. After such an operation, the value of lpBytesReturned is meaningless.

Ah you're right; I've looked at that documentation page many times but somehow always skipped that part.

@ChrisDenton
Copy link
Member Author

That is correct; however, if I'm counting things right, data_len there adds four extra bytes (12 = 8 (MountPointReparseBuffer headers) + 4 (nulls)), which makes me think that the intention was to add those nulls to match some other implementation. Also note that PrintNameOffset has a +1, again looking like it was intended to accommodate the null.

Ah, I see. We really shouldn't be hard coding sizes here. Note though that's only really two null u16 characters, which equates to null terminating each string. Adding null termination makes sense and it would also be better to calculate the sizes and offsets properly rather than using magic numbers.

By the way, just as an interesting observation, PowerShell New-Item uses an empty string for PrintName (like what we do), whereas cmd.exe mklink duplicates the absolute path (without \??\) there. I haven't been able to find out what exactly PrintName is supposed to be for.

The intent of the PrintName was that it provides a user-friendly display path without the application having to calculate it itself (it's not always just removing \\?\). However, this quickly falls apart as the OS does not validate the PrintName in any way so it's vulnerable to malicious applications filling in misleading information. Which makes it useless for its originally intended purpose.

@ChrisDenton ChrisDenton added the O-windows Operating system: Windows label Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants