wasmtime / PR #1855 wasi-common: don't rely on platform d... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1855 wasi-common: don't rely on platform d...

Wasmtime GitHub notifications bot (Jun 10 2020 at 16:28):

ueno opened PR #1855 from wip/dueno/null to master:

If stdio is not inherited nor associated with a file, WasiCtxBuilder tries to open "/dev/null" ("NUL" on Windows) and attach stdio to it. While most platforms today support those device files, it would be
good to avoid unnecessary access to the host device if possible. This patch instead uses a virtual Handle that emulates the "NUL" device.

This is particularly needed when using wasmtime in a restricted environment, such as inside TEE (see https://github.com/enarx/enarx/issues/559 for the context).

Wasmtime GitHub notifications bot (Jun 10 2020 at 17:33):

ueno updated PR #1855 from wip/dueno/null to master:

If stdio is not inherited nor associated with a file, WasiCtxBuilder tries to open "/dev/null" ("NUL" on Windows) and attach stdio to it. While most platforms today support those device files, it would be
good to avoid unnecessary access to the host device if possible. This patch instead uses a virtual Handle that emulates the "NUL" device.

This is particularly needed when using wasmtime in a restricted environment, such as inside TEE (see https://github.com/enarx/enarx/issues/559 for the context).

Wasmtime GitHub notifications bot (Jun 10 2020 at 18:14):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Jun 10 2020 at 18:14):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Jun 10 2020 at 18:14):

kubkon created PR Review Comment:

                let cast_iovlen: types::Size = iov.len().try_into()?;
I think this should bubble up with an error rather than panic, and same below. Oh, and FWIW this should be convertible to crate::wasi::Error without remapping the error types, etc.

Wasmtime GitHub notifications bot (Jun 10 2020 at 18:14):

kubkon created PR Review Comment:

So the reason we've had a PendingEntry::Thunk here was because (if my memory serves me well) OsOther::from_null was fallible and hence it was convenient to save a function returning Result rather than dealing with unwraps, etc., in WasiCtxBuilder::new which is non-fallible. Since I don't see any reason for NullDevice::null_device to be fallible (it always succeeds since it's a virtual device, am I right?), perhaps we could make the fn null_device non-fallible, or better yet, have a more meaningful method such as NullDevice::new as it's constructor, and here, use PendingEntry::Handle instead of PendingEntry::Thunk?

Wasmtime GitHub notifications bot (Jun 10 2020 at 18:14):

kubkon created PR Review Comment:

            .fold(Some(0u32), |len, iov| len.and_then(|x| x.checked_add(iov)))?;

Wasmtime GitHub notifications bot (Jun 11 2020 at 12:24):

ueno updated PR #1855 from wip/dueno/null to master:

If stdio is not inherited nor associated with a file, WasiCtxBuilder tries to open "/dev/null" ("NUL" on Windows) and attach stdio to it. While most platforms today support those device files, it would be
good to avoid unnecessary access to the host device if possible. This patch instead uses a virtual Handle that emulates the "NUL" device.

This is particularly needed when using wasmtime in a restricted environment, such as inside TEE (see https://github.com/enarx/enarx/issues/559 for the context).

Wasmtime GitHub notifications bot (Jun 11 2020 at 12:26):

ueno submitted PR Review.

Wasmtime GitHub notifications bot (Jun 11 2020 at 12:26):

ueno created PR Review Comment:

Thank you for the suggestion! I've rewritten that part along these lines.

Wasmtime GitHub notifications bot (Jun 11 2020 at 12:28):

ueno submitted PR Review.

Wasmtime GitHub notifications bot (Jun 11 2020 at 12:28):

ueno created PR Review Comment:

This and the below seem to be a bit tricky (to a Rust newbie :-), as the error is inside closure. I tried to rewrite this part with try_fold, though it required some manual remapping.

Wasmtime GitHub notifications bot (Jun 11 2020 at 14:26):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Jun 11 2020 at 14:26):

kubkon created PR Review Comment:

Ahh, I missed the fact this was within a map closure. FWIW, in situations like this one, I prefer to use a standard for loop and use ? without the need to remap or carry over the result until the collect/fold step:
let mut total_len = 0;
for iov in iovs {
    let len: types::Size = iov.len().try_into()?;
    total_len = total_len.checked_add(len).ok_or(Errno::Overflow)?;
}
Ok(total_len)

Wasmtime GitHub notifications bot (Jun 11 2020 at 14:30):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (Jun 11 2020 at 14:37):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Jun 11 2020 at 14:47):

ueno updated PR #1855 from wip/dueno/null to master:

If stdio is not inherited nor associated with a file, WasiCtxBuilder tries to open "/dev/null" ("NUL" on Windows) and attach stdio to it. While most platforms today support those device files, it would be
good to avoid unnecessary access to the host device if possible. This patch instead uses a virtual Handle that emulates the "NUL" device.

This is particularly needed when using wasmtime in a restricted environment, such as inside TEE (see https://github.com/enarx/enarx/issues/559 for the context).

Wasmtime GitHub notifications bot (Jun 11 2020 at 14:48):

ueno submitted PR Review.

Wasmtime GitHub notifications bot (Jun 11 2020 at 14:48):

ueno created PR Review Comment:

Indeed, that is much cleaner, thanks!

Wasmtime GitHub notifications bot (Jun 11 2020 at 16:45):

pchickey merged PR #1855.

Last updated: Oct 23 2024 at 20:03 UTC