Stream: git-wasmtime

Topic: wasmtime / PR #4947 fix: check filetype in `path_open`

Wasmtime GitHub notifications bot (Sep 23 2022 at 14:44):

rvolosatovs opened PR #4947 from fix/path_open to main:

Using the directory [open flag] to determine whether the entity being opened is a directory or a file is undefined behavior, since that means that the only way to acquire a usable directory is to specify the flag.

Instead of relying on the [open flag] value to determine the filetype, issue get_path_filestat first to determine it and then perform appropriate operation depending on the value.

Note that this also slightly changes the behavior, where create [open flag] is not dropped anymore, but rather passed through to the open_dir call.

@pchickey seems to be the last person making any significant changes in this method, so assigning him for review

[open flag]: https://github.com/WebAssembly/WASI/blob/main/phases/snapshot/docs.md#-oflags-record

Signed-off-by: Roman Volosatovs <roman@profian.com>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 23 2022 at 14:44):

rvolosatovs edited PR #4947 from fix/path_open to main:

Using the directory [open flag] to determine whether the entity being opened is a directory or a file is undefined behavior, since that means that the only way to acquire a usable directory is to specify the flag.

Instead of relying on the [open flag] value to determine the filetype, issue get_path_filestat first to determine it and then perform appropriate operation depending on the value.

Note that this also slightly changes the behavior, where create [open flag] is not dropped anymore, but rather passed through to the open_dir call.

@pchickey seems to be the last person making any significant changes in this method, so assigning him for review

[open flag]: https://github.com/WebAssembly/WASI/blob/main/phases/snapshot/docs.md#-oflags-record

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 23 2022 at 17:49):

abrown requested pchickey for a review on PR #4947.

Wasmtime GitHub notifications bot (Sep 23 2022 at 17:52):

rvolosatovs updated PR #4947 from fix/path_open to main.

Wasmtime GitHub notifications bot (Sep 23 2022 at 17:53):

rvolosatovs updated PR #4947 from fix/path_open to main.

Wasmtime GitHub notifications bot (Sep 23 2022 at 17:54):

rvolosatovs updated PR #4947 from fix/path_open to main.

Wasmtime GitHub notifications bot (Sep 23 2022 at 18:13):

rvolosatovs edited PR #4947 from fix/path_open to main:

Using the directory [open flag] to determine whether the entity being opened is a directory or a file is undefined behavior, since that means that the only way to acquire a usable directory is to specify the flag.

Instead of relying on the [open flag] value to determine the filetype, issue get_path_filestat first to determine it and then perform appropriate operation depending on the value.

~Note that this also slightly changes the behavior, where create [open flag] is not dropped anymore, but rather passed through to the open_dir call.~

@pchickey seems to be the last person making any significant changes in this method, so assigning him for review

[open flag]: https://github.com/WebAssembly/WASI/blob/main/phases/snapshot/docs.md#-oflags-record

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 23 2022 at 21:37):

rvolosatovs has marked PR #4947 as ready for review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 12:28):

rvolosatovs updated PR #4947 from fix/path_open to main.

Wasmtime GitHub notifications bot (Sep 26 2022 at 13:07):

rvolosatovs updated PR #4947 from fix/path_open to main.

Wasmtime GitHub notifications bot (Sep 26 2022 at 13:18):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 13:18):

rvolosatovs created PR review comment:

Ideally, I would want to fail here if error is not not found, but that would require either cloning the error (to be able to propagate it if it does not match) and then converting it into a types::Errno. Alternatively, it should be possible to downcast the error, but would require matching on all possible error types, which seems like the wrong thing to do as well.
Is there a reasonable way to do that? I'm looking for Error::not_found(&self) -> bool
Discard the error instead and preserve the original behavior.

Wasmtime GitHub notifications bot (Sep 26 2022 at 13:19):

rvolosatovs edited PR review comment.

Wasmtime GitHub notifications bot (Sep 26 2022 at 13:40):

rvolosatovs updated PR #4947 from fix/path_open to main.

Wasmtime GitHub notifications bot (Sep 26 2022 at 17:49):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 17:49):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 17:49):

pchickey created PR review comment:

I think it is reasonable to downcast_ref and match for an ErrorKind::Noent, and if that fails downcast to a std::io::Error with the appropriate check there as well. This is a big janky, and I think if I had to redesign wasi-common I probably wouldn't use a fully dynamic error type and instead make enum Error { Std(std::io::Error), Wasi(ErrorKind) } or something like that, but its been a while since I had this system in my head so I'm not sure why I didn't do that in the first place.

Wasmtime GitHub notifications bot (Sep 26 2022 at 21:09):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 21:09):

sunfishcode created PR review comment:

I am concerned about this get_path_filestat, because it does a second path lookup. There's a window for a race condition here, where a directory could be replaced by something which is not a directory. I don't yet have a clear view of the consequences of the open succeeding on an unexpected type, so this is something I want to investigate more.

I wonder if it would be possible to instead do something like:

• Attempt to open with open_dir; if that works, create a DirEntry entry.
• If that fails with Notdir, open it with regular open.
  Would that work?

I know that this is redundant on POSIX hosts, and possibly we could optimize things even more there, but this code is currently written to support Windows also.

Wasmtime GitHub notifications bot (Sep 27 2022 at 01:37):

sunfishcode created PR review comment:

I now have an implementation of this here: https://github.com/bytecodealliance/wasmtime/pull/4967

It uses the same testcase as this PR, so it fixes the same bug, but does so in a way that doesn't require doing a stat followed by a separate open. There's more room for improvement beyond that, but hopefully that's a good step.

Wasmtime GitHub notifications bot (Sep 27 2022 at 01:37):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Apr 21 2023 at 17:33):

pchickey closed without merge PR #4947.

Last updated: Nov 22 2024 at 17:03 UTC