Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 104 additions & 1 deletion ext/src/ruby_api/wasi_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@ use crate::error;
use crate::helpers::OutputLimitedBuffer;
use magnus::{
class, function, gc::Marker, method, typed_data::Obj, value::Opaque, DataTypeFunctions, Error,
Module, Object, RArray, RHash, RString, Ruby, TryConvert, TypedData,
Module, Object, RArray, RHash, RString, Ruby, Symbol, TryConvert, TypedData,
};
use rb_sys::ruby_rarray_flags::RARRAY_EMBED_FLAG;
use std::cell::RefCell;
use std::fs;
use std::path::Path;
use std::{fs::File, path::PathBuf};
use wasmtime_wasi::p2::pipe::MemoryInputPipe;
use wasmtime_wasi::p2::{OutputFile, WasiCtx, WasiCtxBuilder};
use wasmtime_wasi::preview1::WasiP1Ctx;
use wasmtime_wasi::{DirPerms, FilePerms};

enum ReadStream {
Inherit,
Expand Down Expand Up @@ -51,6 +54,7 @@ struct WasiConfigInner {
env: Option<Opaque<RHash>>,
args: Option<Opaque<RArray>>,
deterministic: bool,
mapped_directories: Option<Opaque<RArray>>,
}

impl WasiConfigInner {
Expand All @@ -70,6 +74,9 @@ impl WasiConfigInner {
if let Some(v) = self.args.as_ref() {
marker.mark(*v);
}
if let Some(v) = self.mapped_directories.as_ref() {
marker.mark(*v);
}
}
}

Expand Down Expand Up @@ -233,6 +240,40 @@ impl WasiConfig {
rb_self
}

/// @yard
/// Set mapped directory for host path and guest path.
/// @param host_path [String]
/// @param guest_path [String]
/// @param dir_perms [Symbol] Directory permissions, one of :read, :mutate, or :all
/// @param file_perms [Symbol] File permissions, one of :read, :write, or :all
/// @def set_mapped_directory(host_path, guest_path, dir_perms, file_perms)
/// @return [WasiConfig] +self+
pub fn set_mapped_directory(
rb_self: RbSelf,
host_path: RString,
guest_path: RString,
dir_perms: Symbol,
file_perms: Symbol,
) -> RbSelf {
let mapped_directory = RArray::new();
mapped_directory.push(host_path).unwrap();
mapped_directory.push(guest_path).unwrap();
mapped_directory.push(dir_perms).unwrap();
mapped_directory.push(file_perms).unwrap();

let init_directory = RArray::new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the creation of this RArray be moved under the if on line 267? Else we'd be creating an array even though it won't be needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we consider storing these in a Vec<SomeStruct> instead of nested RArrays?

Pros:

  • Sidesteps the refcell+alloc issue altogether (the top-level RArray is gone).
  • Fewer Ruby allocations: no need to alloc an Array to store the arguments for each call.
  • Errors converting Ruby args into WasiCtxBuilder.preopened_dir-compatible args will show up immediately, not delayed in a future call. Tiny detail, but makes errors easier to debug.

Cons:

  • Requires implementing such struct to hold the arguments, and marking the string it contains.
  • Something else I'm not seeing?

I didn't mean to hijack the PR, feel free to ignore if not useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the suggestion is reasonable, personally the main advantage that I see, is not having to incur in the overhead of creating a Ruby array each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I switched to using a Vec and there are still no garbage collection problems.


let mut inner = rb_self.inner.borrow_mut();
if inner.mapped_directories.is_none() {
inner.mapped_directories = Some(init_directory.into());
}

let ruby = Ruby::get().unwrap();
let mapped_directories = ruby.get_inner(inner.mapped_directories.unwrap());
mapped_directories.push(mapped_directory).unwrap();
rb_self
}

pub fn build_p1(&self, ruby: &Ruby) -> Result<WasiP1Ctx, Error> {
let mut builder = self.build_impl(ruby)?;
let ctx = builder.build_p1();
Expand Down Expand Up @@ -317,6 +358,63 @@ impl WasiConfig {
deterministic_wasi_ctx::add_determinism_to_wasi_ctx_builder(&mut builder);
}

if let Some(mapped_directories) = inner.mapped_directories.as_ref() {
for item in unsafe { ruby.get_inner(*mapped_directories).as_slice() } {
let mapped_directory = RArray::try_convert(*item)?;
if mapped_directory.len() == 4 {
let host_path =
RString::try_convert(mapped_directory.entry(0)?)?.to_string()?;
let guest_path =
RString::try_convert(mapped_directory.entry(1)?)?.to_string()?;
let dir_perms = Symbol::from_value(mapped_directory.entry(2)?)
.unwrap()
.name()?;
let file_perms = Symbol::from_value(mapped_directory.entry(3)?)
.unwrap()
.name()?;

let host_path_dir = Path::new(&host_path);
let guest_path_path = guest_path.as_str();

// Convert to FilePerms and DirPerms enums
let dir_perms_flags;
match dir_perms {
std::borrow::Cow::Borrowed("read") => dir_perms_flags = DirPerms::READ,
std::borrow::Cow::Borrowed("mutate") => dir_perms_flags = DirPerms::MUTATE,
std::borrow::Cow::Borrowed("all") => dir_perms_flags = DirPerms::all(),
_ => {
return Err(error!(
"Invalid dir_perms: {}. Use one of :read, :mutate, or :all",
dir_perms
))
}
}

let file_perms_flags;
match file_perms {
std::borrow::Cow::Borrowed("read") => file_perms_flags = FilePerms::READ,
std::borrow::Cow::Borrowed("write") => file_perms_flags = FilePerms::WRITE,
std::borrow::Cow::Borrowed("all") => file_perms_flags = FilePerms::all(),
_ => {
return Err(error!(
"Invalid file_perms: {}. Use one of :read, :write, or :all",
file_perms
))
}
}

builder
.preopened_dir(
host_path_dir,
guest_path_path,
dir_perms_flags,
file_perms_flags,
)
.map_err(|e| error!("{}", e))?;
}
}
}

Ok(builder)
}
}
Expand Down Expand Up @@ -355,5 +453,10 @@ pub fn init() -> Result<(), Error> {

class.define_method("set_argv", method!(WasiConfig::set_argv, 1))?;

class.define_method(
"set_mapped_directory",
method!(WasiConfig::set_mapped_directory, 4),
)?;

Ok(())
}
Binary file added spec/fixtures/wasi-fs-p2.wasm
Binary file not shown.
Binary file added spec/fixtures/wasi-fs.wasm
Binary file not shown.
2 changes: 2 additions & 0 deletions spec/fixtures/wasi-fs/.cargo/config.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[build]
target = "wasm32-wasip1"
10 changes: 10 additions & 0 deletions spec/fixtures/wasi-fs/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "wasi-fs"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[workspace]

[dependencies]
14 changes: 14 additions & 0 deletions spec/fixtures/wasi-fs/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
Example WASI program used to test WASI preopened directories

To update:

```shell
cargo build --release && \
wasm-opt -O \
--enable-bulk-memory \
target/wasm32-wasip1/release/wasi-fs.wasm \
-o ../wasi-fs.wasm && \
cargo build --target=wasm32-wasip2 --release && \
cp target/wasm32-wasip2/release/wasi-fs.wasm \
../wasi-fs-p2.wasm
```
18 changes: 18 additions & 0 deletions spec/fixtures/wasi-fs/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use std::io::{Write, Read};
use std::fs::File;

fn main() {
let args: Vec<String> = std::env::args().collect();

let counter_file = File::open(&args[1]).expect("failed to open counter file");

let mut counter_str = String::new();
counter_file.take(100).read_to_string(&mut counter_str)
.expect("failed to read counter file");
let mut counter: u32 = counter_str.trim().parse().expect("failed to parse counter");

counter += 1;

let mut counter_file = File::create(&args[1]).expect("failed to create counter file");
write!(counter_file, "{}", counter).expect("failed to write counter file");
}
111 changes: 111 additions & 0 deletions spec/unit/wasi_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ module Wasmtime
# Compile module only once for speed
@compiled_wasi_module = @engine.precompile_module(IO.binread("spec/fixtures/wasi-debug.wasm"))
@compiled_wasi_deterministic_module = @engine.precompile_module(IO.binread("spec/fixtures/wasi-deterministic.wasm"))
@compiled_wasi_fs_module = @engine.precompile_module(IO.binread("spec/fixtures/wasi-fs.wasm"))

@compiled_wasi_component = @engine.precompile_component(IO.binread("spec/fixtures/wasi-debug-p2.wasm"))
@compiled_wasi_deterministic_component = @engine.precompile_component(IO.binread("spec/fixtures/wasi-deterministic-p2.wasm"))
@compiled_wasi_fs_component = @engine.precompile_component(IO.binread("spec/fixtures/wasi-fs-p2.wasm"))
end

describe "Linker.new" do
Expand Down Expand Up @@ -233,13 +235,103 @@ module Wasmtime
end
end
end

it "writes to mapped directory" do
Dir.mkdir(tempfile_path("tmp"))
File.write(tempfile_path(File.join("tmp", "counter")), "0")

wasi_config = WasiConfig.new
.set_argv(["wasi-fs", "/tmp/counter"])
.set_mapped_directory(tempfile_path("tmp"), "/tmp", :all, :all)

expect { run_fs.call(wasi_config) }.not_to raise_error

expect(File.read(tempfile_path(File.join("tmp", "counter")))).to eq("1")
end

it "fails to write to mapped directory if not permitted" do
Dir.mkdir(tempfile_path("tmp"))
File.write(tempfile_path(File.join("tmp", "counter")), "0")

stderr_str = ""
wasi_config = WasiConfig.new
.set_argv(["wasi-fs", "/tmp/counter"])
.set_stderr_buffer(stderr_str, 40000)
.set_mapped_directory(tempfile_path("tmp"), "/tmp", :read, :read)

expect { run_fs.call(wasi_config) }.to raise_error do |error|
expect(error).to be_a(Wasmtime::Error)
end

expect(stderr_str).to match(/failed to create counter file/)

expect(File.read(tempfile_path(File.join("tmp", "counter")))).to eq("0")
end

it "fails to read from mapped directory if not permitted" do
Dir.mkdir(tempfile_path("tmp"))
File.write(tempfile_path(File.join("tmp", "counter")), "0")

stderr_str = ""
wasi_config = WasiConfig.new
.set_argv(["wasi-fs", "/tmp/counter"])
.set_stderr_buffer(stderr_str, 40000)
.set_mapped_directory(tempfile_path("tmp"), "/tmp", :mutate, :write)

expect { run_fs.call(wasi_config) }.to raise_error do |error|
expect(error).to be_a(Wasmtime::Error)
end

expect(stderr_str).to match(/failed to open counter file/)

expect(File.read(tempfile_path(File.join("tmp", "counter")))).to eq("0")
end

it "fails to access non-mapped directories" do
Dir.mkdir(tempfile_path("tmp"))
File.write(tempfile_path(File.join("tmp", "counter")), "0")

stderr_str = ""
wasi_config = WasiConfig.new
.set_argv(["wasi-fs", File.join(tempfile_path("tmp"), "counter")])
.set_stderr_buffer(stderr_str, 40000)

expect { run_fs.call(wasi_config) }.to raise_error do |error|
expect(error).to be_a(Wasmtime::Error)
end

expect(stderr_str).to match(/failed to find a pre-opened file descriptor/)

expect(File.read(tempfile_path(File.join("tmp", "counter")))).to eq("0")
end

it "does not accept an invalid host path" do
wasi_config = WasiConfig.new
.set_mapped_directory(tempfile_path("tmp"), "/tmp", :all, :all)

expect { run_fs.call(wasi_config) }.to raise_error do |error|
expect(error).to be_a(Wasmtime::Error)
# error message is os-specific
end
end

it "does not accept invalid permissions" do
wasi_config = WasiConfig.new
.set_mapped_directory(tempfile_path("tmp"), "/tmp", :mutate, :invalid_permission)

expect { run_fs.call(wasi_config) }.to raise_error do |error|
expect(error).to be_a(Wasmtime::Error)
expect(error.message).to match(/Invalid file_perms: invalid_permission. Use one of :read, :write, or :all/)
end
end
end

describe "WasiConfig preview 1" do
it_behaves_like WasiConfig do
let(:run) { method(:run_wasi_module) }
let(:wasi_env) { method(:wasi_module_env) }
let(:run_deterministic) { method(:run_wasi_module_deterministic) }
let(:run_fs) { method(:run_wasi_module_fs) }
end
end

Expand All @@ -248,6 +340,7 @@ module Wasmtime
let(:run) { method(:run_wasi_component) }
let(:wasi_env) { method(:wasi_component_env) }
let(:run_deterministic) { method(:run_wasi_component_deterministic) }
let(:run_fs) { method(:run_wasi_component_fs) }
end
end

Expand All @@ -272,6 +365,13 @@ def run_wasi_module_deterministic(wasi_config)
.invoke("_start")
end

def run_wasi_module_fs(wasi_config)
linker = Linker.new(@engine)
WASI::P1.add_to_linker_sync(linker)
store = Store.new(@engine, wasi_p1_config: wasi_config)
linker.instantiate(store, Module.deserialize(@engine, @compiled_wasi_fs_module)).invoke("_start")
end

def wasi_module_env
stdout_file = tempfile_path("stdout")

Expand Down Expand Up @@ -318,6 +418,17 @@ def run_wasi_component_deterministic(wasi_config)
).call_run(store)
end

def run_wasi_component_fs(wasi_config)
linker = Component::Linker.new(@engine)
WASI::P2.add_to_linker_sync(linker)
store = Store.new(@engine, wasi_config: wasi_config)
Component::WasiCommand.new(
store,
Component::Component.deserialize(@engine, @compiled_wasi_fs_component),
linker
).call_run(store)
end

def tempfile_path(name)
File.join(tmpdir, name)
end
Expand Down
Loading