marmistrz requested peterhuene, kubkon, and sunfishcode for a review on PR #1359.
marmistrz requested peterhuene, kubkon, and sunfishcode for a review on PR #1359.
marmistrz requested peterhuene, kubkon, and sunfishcode for a review on PR #1359.
marmistrz opened PR #1359 from dir_symlink
to master
:
Based from my experience,
symlink_file
will not return an error if we try to create a file symlink to a directory. I used the following Rust program to verify it:use std::env; use std::io::Result; use std::os::windows::fs::symlink_file; pub(crate) fn path_symlink(src: &str, dst: &str) -> Result<()> { // try creating a file symlink symlink_file(src, dst).map_err(|e| { println!("ERROR: {}", e); e }) } fn main() { let args: Vec<_> = env::args().collect(); path_symlink(&args[1], &args[2]).expect("path_symlink failed"); }If
dir
is an existing directory, and the binary as executed withcargo run dir target
, the program will succeed and create a file symlink.For details on file vs. directory symlinks on Windows see #1358.
@kubkon: I'm also unsure about the correctness of the following error mappings:
https://github.com/marmistrz/wasmtime/blob/1c61210025c00f0d397be86d752717ab20642f53/crates/wasi-common/src/sys/windows/hostcalls_impl/fs.rs#L527-L536
In which cases are they expected to be encountered? What's the scenario they're there for?
peterhuene submitted PR Review.
marmistrz updated PR #1359 from dir_symlink
to master
:
Based from my experience,
symlink_file
will not return an error if we try to create a file symlink to a directory. I used the following Rust program to verify it:use std::env; use std::io::Result; use std::os::windows::fs::symlink_file; pub(crate) fn path_symlink(src: &str, dst: &str) -> Result<()> { // try creating a file symlink symlink_file(src, dst).map_err(|e| { println!("ERROR: {}", e); e }) } fn main() { let args: Vec<_> = env::args().collect(); path_symlink(&args[1], &args[2]).expect("path_symlink failed"); }If
dir
is an existing directory, and the binary as executed withcargo run dir target
, the program will succeed and create a file symlink.For details on file vs. directory symlinks on Windows see #1358.
@kubkon: I'm also unsure about the correctness of the following error mappings:
https://github.com/marmistrz/wasmtime/blob/1c61210025c00f0d397be86d752717ab20642f53/crates/wasi-common/src/sys/windows/hostcalls_impl/fs.rs#L527-L536
In which cases are they expected to be encountered? What's the scenario they're there for?
kubkon created PR Review Comment:
Is this change only targeting the old snapshot?
kubkon submitted PR Review.
marmistrz updated PR #1359 from dir_symlink
to master
:
Based from my experience,
symlink_file
will not return an error if we try to create a file symlink to a directory. I used the following Rust program to verify it:use std::env; use std::io::Result; use std::os::windows::fs::symlink_file; pub(crate) fn path_symlink(src: &str, dst: &str) -> Result<()> { // try creating a file symlink symlink_file(src, dst).map_err(|e| { println!("ERROR: {}", e); e }) } fn main() { let args: Vec<_> = env::args().collect(); path_symlink(&args[1], &args[2]).expect("path_symlink failed"); }If
dir
is an existing directory, and the binary as executed withcargo run dir target
, the program will succeed and create a file symlink.For details on file vs. directory symlinks on Windows see #1358.
@kubkon: I'm also unsure about the correctness of the following error mappings:
https://github.com/marmistrz/wasmtime/blob/1c61210025c00f0d397be86d752717ab20642f53/crates/wasi-common/src/sys/windows/hostcalls_impl/fs.rs#L527-L536
In which cases are they expected to be encountered? What's the scenario they're there for?
marmistrz submitted PR Review.
marmistrz created PR Review Comment:
Whoops,
git rebase
went wrong. It should be fine now.
marmistrz edited PR Review Comment.
marmistrz updated PR #1359 from dir_symlink
to master
:
Based from my experience,
symlink_file
will not return an error if we try to create a file symlink to a directory. I used the following Rust program to verify it:use std::env; use std::io::Result; use std::os::windows::fs::symlink_file; pub(crate) fn path_symlink(src: &str, dst: &str) -> Result<()> { // try creating a file symlink symlink_file(src, dst).map_err(|e| { println!("ERROR: {}", e); e }) } fn main() { let args: Vec<_> = env::args().collect(); path_symlink(&args[1], &args[2]).expect("path_symlink failed"); }If
dir
is an existing directory, and the binary as executed withcargo run dir target
, the program will succeed and create a file symlink.For details on file vs. directory symlinks on Windows see #1358.
@kubkon: I'm also unsure about the correctness of the following error mappings:
https://github.com/marmistrz/wasmtime/blob/1c61210025c00f0d397be86d752717ab20642f53/crates/wasi-common/src/sys/windows/hostcalls_impl/fs.rs#L527-L536
In which cases are they expected to be encountered? What's the scenario they're there for?
kubkon submitted PR Review.
kubkon created PR Review Comment:
Hmm, I'm somewhat confused here. Why do we need this check? Or put it another way, if we're doing this check already here, is creating a dir symlink on error
ERROR_NOT_A_REPARSE_POINT
still needed? Is the current approach of try and fail and try something else not working?Some context on why we've originally had
symlink_dir
call after a failingsymlink_file
call. As @sunfishcode explained a long time ago, from atomicity point of view, it seems virtually always better to try and fail and then correct, rather than invoke another syscall (in this case viafs::metadata
) which would check what the resource (in this case the symlink) is since in between the check and actual call tosymlink_file
etc. the path might change, or whatnot.
marmistrz created PR Review Comment:
I was unable to encounter
ERROR_NOT_A_REPARSE_POINT
while usingsymlink_file
with the symlink target being a directory. As far as I have tested, the current approach of try and fail doesn't work and a file symlink will always be created.I agree that try and fail would be preferable, but it appears that
CreateSymbolicLinkA
doesn't check if the symlink target is a file or directory. I don't know ifERROR_NOT_A_REPARSE_POINT
may be returned byCreateSymbolicLinkA
at all.
marmistrz submitted PR Review.
kubkon submitted PR Review.
kubkon created PR Review Comment:
Hmm, sounds like a bug then. I thought that
ERROR_NOT_A_REPARSE_POINT
played some role in this, but alas I cannot remember the exact test conditions now. Anyhow, I thought it was covered by our test programs, but from what I understand, you've proved that they were not properly testing this behaviour on Windows? Would the test work fine if we got ridERROR_NOT_A_REPARSE_POINT
match altogether? And would they still work without both the match and your upfront check?
kubkon created PR Review Comment:
Oh, and here's another thought. What would happen if we reversed the order of ops, i.e., started with
symlink_dir
, and if failed, triedsymlink_file
. Would that make it possible to apply the "try-catch" approach rather than an upfront check?
kubkon submitted PR Review.
marmistrz created PR Review Comment:
Trying to create a symlink to a file using
symlink_dir
succeeds too.
marmistrz submitted PR Review.
kubkon created PR Review Comment:
Interesting, but then the symlinks are obviously invalid if the user would try to act on them in say Windows Explorer or whatnot?
kubkon submitted PR Review.
marmistrz submitted PR Review.
marmistrz created PR Review Comment:
Yes, Windows Explorer will show an error, claiming that the directory is invalid.
marmistrz updated PR #1359 from dir_symlink
to master
:
Based from my experience,
symlink_file
will not return an error if we try to create a file symlink to a directory. I used the following Rust program to verify it:use std::env; use std::io::Result; use std::os::windows::fs::symlink_file; pub(crate) fn path_symlink(src: &str, dst: &str) -> Result<()> { // try creating a file symlink symlink_file(src, dst).map_err(|e| { println!("ERROR: {}", e); e }) } fn main() { let args: Vec<_> = env::args().collect(); path_symlink(&args[1], &args[2]).expect("path_symlink failed"); }If
dir
is an existing directory, and the binary as executed withcargo run dir target
, the program will succeed and create a file symlink.For details on file vs. directory symlinks on Windows see #1358.
@kubkon: I'm also unsure about the correctness of the following error mappings:
https://github.com/marmistrz/wasmtime/blob/1c61210025c00f0d397be86d752717ab20642f53/crates/wasi-common/src/sys/windows/hostcalls_impl/fs.rs#L527-L536
In which cases are they expected to be encountered? What's the scenario they're there for?
kubkon merged PR #1359.
Last updated: Dec 23 2024 at 12:05 UTC