-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
In wasmtime CLI, passing --tcplisten argument breaks --dir #3936
Comments
CC @haraldh. It's possible this is related to how WASI programs discover the preopened sockets. Would it work to do the This is an area where, in the future, Typed Main will help: instead of having WASI programs search through the file descriptor space to find preopens, we ideally want them provided explicitly. |
@haraldh is this something you might be able to look into? Addressing this looks like it'd help make |
yes, will do |
Ok, here is the analysis.
So with a WASI change like this: diff --git a/phases/snapshot/witx/typenames.witx b/phases/snapshot/witx/typenames.witx
index 893e5b2..6aef907 100644
--- a/phases/snapshot/witx/typenames.witx
+++ b/phases/snapshot/witx/typenames.witx
@@ -730,6 +730,7 @@
(enum (@witx tag u8)
;;; A pre-opened directory.
$dir
+ $listen_socket
)
)
@@ -741,10 +742,19 @@
)
)
+;;; The contents of a $prestat when type is `preopentype::listen_socket`.
+(typename $prestat_listen_socket
+ (record
+ ;;; The length of the address string for use with `fd_prestat_socket_addr`.
+ (field $pr_addr_len $size)
+ )
+)
+
;;; Information about a pre-opened capability.
(typename $prestat
(union (@witx tag $preopentype)
$prestat_dir
+ $prestat_listen_socket
)
) we could fix diff --git a/crates/wasi-common/src/snapshots/preview_0.rs b/crates/wasi-common/src/snapshots/preview_0.rs
index 96f86820b..8bdc2039a 100644
--- a/crates/wasi-common/src/snapshots/preview_0.rs
+++ b/crates/wasi-common/src/snapshots/preview_0.rs
@@ -198,6 +198,7 @@ impl From<snapshot1_types::Prestat> for types::Prestat {
fn from(p: snapshot1_types::Prestat) -> types::Prestat {
match p {
snapshot1_types::Prestat::Dir(d) => types::Prestat::Dir(d.into()),
+ snapshot1_types::Prestat::ListenSocket(_) => todo!(),
}
}
}
diff --git a/crates/wasi-common/src/snapshots/preview_1.rs b/crates/wasi-common/src/snapshots/preview_1.rs
index 9c6f372d3..758b9da61 100644
--- a/crates/wasi-common/src/snapshots/preview_1.rs
+++ b/crates/wasi-common/src/snapshots/preview_1.rs
@@ -554,13 +554,26 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
async fn fd_prestat_get(&mut self, fd: types::Fd) -> Result<types::Prestat, Error> {
let table = self.table();
- let dir_entry: &DirEntry = table.get(u32::from(fd)).map_err(|_| Error::badf())?;
- if let Some(ref preopen) = dir_entry.preopen_path() {
- let path_str = preopen.to_str().ok_or_else(|| Error::not_supported())?;
- let pr_name_len = u32::try_from(path_str.as_bytes().len())?;
- Ok(types::Prestat::Dir(types::PrestatDir { pr_name_len }))
+ if let Ok(dir_entry) = table.get::<DirEntry>(u32::from(fd)) {
+ if let Some(ref preopen) = dir_entry.preopen_path() {
+ let path_str = preopen.to_str().ok_or_else(|| Error::not_supported())?;
+ let pr_name_len = u32::try_from(path_str.as_bytes().len())?;
+ Ok(types::Prestat::Dir(types::PrestatDir { pr_name_len }))
+ } else {
+ Err(Error::not_supported().context("file is not a preopen"))
+ }
+ } else if let Ok(file_entry) = table.get_mut::<FileEntry>(u32::from(fd)) {
+ let fdstat = file_entry.get_fdstat().await?;
+ if matches!(fdstat.filetype, FileType::SocketStream) {
+ let pr_addr_len = 0; // TODO
+ Ok(types::Prestat::ListenSocket(types::PrestatListenSocket {
+ pr_addr_len,
+ }))
+ } else {
+ Err(Error::not_supported().context("file is not a preopen"))
+ }
} else {
- Err(Error::not_supported().context("file is not a preopen"))
+ Err(Error::badf())
}
} which would fix the code of this issue. |
Existing libraries and applications use `fd_prestat_get()` to find out about preopened directories. To not break the old logic of those, if preopened sockets are added, push the directories first. See bytecodealliance#3936 Signed-off-by: Harald Hoyer <[email protected]>
The quick "band-aid" fix is #3995 |
Thanks for looking into this, @haraldh! Just to clarify, how is application code meant to know which file descriptor numbers to pass to Is there a recommended pattern of calls for application code to discover the available TCP listener file descriptors? cc @sunfishcode |
Using the |
A combination of |
Thanks for the info! That's really useful.
Does the "band-aid" fix (#3995) satisfy this condition? I did try out the patch given above and observed that it did still supply listener FDs starting from 3, even when there's also a directory preopen. |
Or we have to increment the |
The "band-aid" fix violates this condition... The fix with the WASI spec change satisfies it. |
@bjorn3 One problem with |
Fds 0, 1 and 2 are reserved for stdin, stdout and stderr respectively. Wasi doesn't allow them to be absent AFAIK. |
Anyway, to get into this situation, you have to deliberately use the new command line features or the new
So, instead of |
@bjorn3 I don't see stdio in the snapshot at all. |
What snapshot? |
Both the wasi impl in wasmtime and wasi-libc assume the first three fds are stdio: wasmtime/crates/wasi-common/src/ctx.rs Lines 35 to 37 in 76f7cde
|
Hi, thanks for raising this since I hit the same problem when implementing a web server that serve static files. I am wondering if there is a consensus on how to fix it? Could be the case based on this discussion above but I am not 100% sure. @bjorn3 said:
Then @haraldh said:
So is it correct to assume that it is possible to fix this issue without extending the WASI spec, in a compliant way with man sd_listen_fds (preopened directories must be after the preopened sockets) just by refining how Wasmtime handles this? Happy to try crafting a PR with proper guidance. |
But it isn't in the standard. Implementations can assume anything they want. It doesn't make it standard behavior. |
Anything that wants to use wasi-libc implicitly assumes this. Assemblyscript also assumes it (https://github.com/AssemblyScript/assemblyscript/blob/d884ac8032b2bfa0caf26d4dc11d99c5a9543c13/std/assembly/wasi/index.ts#L57) Nodejs also uses 0, 1 and 2 for stdin, stdout and stderr by default. Because of this IMO it should just be added to the actual wasi standard as it is already de-facto standard, just not de-jure. |
@bjorn3 But this is an example of a host feature "bleeding" into the spec. In Enarx, the host stdio is untrusted and shouldn't be connected to the encrypted guest wasm instance. I do not thing we should presume that just because everyone assumes it that it should be added to WASI uncritically. |
It isn't required that the host stdio is connected to the guest stdio. The guest can have /dev/null or a file on which every operations fails as stdio if there is no stdio to pass through. |
WASI is in the process of moving to interface types and Typed Main, at which point I expect we'll completely overhaul the way file descriptors are passed into programs. The accept feature here is about enabling certain functionality with minimal changes to the current infrastructure. Within the current infrastructure, fds 0,1,2 are effectively reserved for stdin, stdout, stderr. wasi-libc assumes this. This isn't how POSIX would do things, or how Linux would do them, but it's how the current infrastructure works, so the way to make things work with minimal changes is just to teach everything that 0,1,2 are reserved for stdin, stdout, and stderr. |
Hi, is there any progress on this issue? I would like to use |
@sunfishcode is there any resolution we might work toward here? This continues to break a few things. |
it appears that #3995 should fix it, but... |
@squillace is it possible to use components instead and use the |
we'll have a look to see whether the wasi-sockets proposal is ready for freddy. In general, there'll be a choice that language runtimes make here, so for them this is a fork in the road, so to speak.... |
Hi, @alexcrichton , currently we use the sock_accept importing the function like this in our C code:
How should I use components to replace it? Is there any documentation that I can follow? |
@thaystg I think that, unfortunately, the answer won't be so simple. If using the component model and/or preview 2 you wouldn't have access to that API exactly but you would have access to other If you're interested though it's theoretically possible to get it working. Wasmtime has an in-guest adapter which translates preview1 to preview2, and this is how binaries don't have to explicitly update to preview2 yet to make use of it. The way this works is that the adapter exports functions like To set expectations, though, I don't expect that filling out the adapter for socket-related functionality is going to be an easy task. It's god a nontrivial design component to it along with implementation concerns too. That's all surmountable in the technical sense which is why I'm mentioning this. I'm not aware of anyone else who's lined up to implement this, however, and so far I don't believe this is considered a blocker or todo item for the preview2 timeline, only a nice-to-have if someone is interested in implementing it. |
Hey @alexcrichton thanks; so to sum up: Re: #3936 (comment) we should jump on the components train but in the components train we have to implement something that wasmtime experts should probably design. Do I have this correct? If so, I'd be a bit more interested hearing about Your take? |
Almost yeah, although the component part doesn't require any work on Wasmtime's side, but it may require more work on y'all's side if you're not already using components. I can try to summarize as well a bit if that helps. There's more-or-less three different ways to go about solving this: Fix this single issue in isolationThere's quite a bit of discussion on this thread, and for example I haven't reviewed the above possible fix mentioned. Fixing this issue in isolation will require confirming that it actually works with wasi-libc which seems like a hazard given the above discussion. I haven't dug into this myself because sockets in preview1 are not something we'd like to breathe more life into at this time. Instead we're looking ideally to encourage users to use the preview2 implementation where possible. That being said from a perspective of "I just want something working" this is probably the easiest route since it has nothing to do with components and it's probably just taking existing stuff and wiring it together a bit differently. Implement
|
Thanks, @alexcrichton, I appreciate this. We are intending to target components directly, so skating to where the puck WILL be is better, I think, though it will require us to do more work in the shorter term that we eventually intended to do. @SteveSandersonMS and @thaystg let's understand how to tackle this and come back to the conversation. |
Is there a workaround to use |
Test Case
testcase.zip
Steps to Reproduce
main.wasm
from the test case zip file. Or compile it yourself using WASI SDK and the following source code, e.g. via~/wasi-sdk/bin/clang main.c --sysroot ~/wasi-sdk/share/wasi-sysroot -o main.wasm
if you have WASI SDK at~/wasi-sdk
.wasmtime main.wasm --dir=.
and observe it print a directory listingwasmtime main.wasm --dir=. --tcplisten=127.0.0.1:9000
and observe it fail to do so (it will print Couldn't open the directory)Expected Results
I expect that
--tcplisten
should not break the directory mapping.Actual Results
Passing
--tcplisten
makes it behave as if you didn't pass--dir=.
.Versions and Environment
Wasmtime version or commit: 0.35.1
Operating system: linux
Architecture: x86_64
Extra Info
Other than this,
--tcplisten
works brilliantly and I've been able to implement a nontrivial web server application using this flag and the newsock_accept
API.The text was updated successfully, but these errors were encountered: