Skip to content
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

Assign the file system when calling function #563

Closed
knqyf263 opened this issue May 16, 2022 · 14 comments · Fixed by #571
Closed

Assign the file system when calling function #563

knqyf263 opened this issue May 16, 2022 · 14 comments · Fixed by #571
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@knqyf263
Copy link
Contributor

I'd like to pass the different filesystem when calling a function as below.

	callback := mod.ExportedFunction("callback")
	filepath.WalkDir("root", func(path string, d fs.DirEntry, err error) error {
		if d.IsDir() && (path == "appA" || path == "appB") {
			callback.Call(ctx, wazero.WithFS(os.DirFS(path)))
		}
		return nil
	})

This is because we want wasm to be able to access only files under appA and appB. As far as I know, it is not supported now. It would be like the following in wazero.

	code, err := r.CompileModule(ctx, wasmCode, wazero.NewCompileConfig())
	if err != nil {...}

	filepath.WalkDir("root", func(path string, d fs.DirEntry, err error) error {
		if d.IsDir() && (path == "appA" || path == "appB") {
			rooted := os.DirFS(path)
			config := wazero.NewModuleConfig().WithFS(rooted)
			mod, err := r.InstantiateModule(ctx, code, config)
			if err != nil {...}
			defer mod.Close(ctx)

			callback := mod.ExportedFunction("callback")
			callback.Call(ctx)
		}
		return nil
	})

Is this the best way? I am afraid of the penalty of initializing the module every time since it is called frequently.

@mathetake mathetake added enhancement New feature or request question Further information is requested labels May 16, 2022
@mathetake
Copy link
Member

oh yeah, this sounds reasonable to me and should be possible. Assigning @codefromthecrypt as this is an API-related thing!

@codefromthecrypt
Copy link
Contributor

@knqyf263 would context-based configuration work for you? This feels like the solution here, or at least a workaround. Something like https://pkg.go.dev/github.com/tetratelabs/wazero/experimental#Sys

@knqyf263
Copy link
Contributor Author

Do you mean you will add a new function to the Sys interface?

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented May 16, 2022

I was thinking another key, actually experimental.FSKey *

What would happen is that any call in the stack would have this as its FS, unless overridden again. Ex. wasm would see this FS, so would any host function call, recursively, unless some host function overrode the key again.

fn.Call(context.WithValue(ctx, experimental.FSKey{}, os.DirFS(path)), ....)

* We will clean up the "experimental" namespace after implementing WASI snapshot 2 #263 and/or adding the new FS type #390. I think doing too much now would probably end up drifting later due to these anyway, so experimental is likely not the worst way to describe it and meanwhile we can this out to you and figure out how well it works.

@codefromthecrypt
Copy link
Contributor

oh.. (sorry jet lag) @knqyf263 are you also asking to have host functions read the FS? Maybe you can clarify what you need from this below:

  1. have host functions read the current FS (currently only wasm can)
  2. override the current FS for downstream calls (currently only via module instantiation)
  • note we have both root and working directory FS at the moment

@codefromthecrypt
Copy link
Contributor

ps overriding the FS entirely (similar to how it would be done in moduleconfig) is probably easier impl, though we'd have to add a security disclaimer anyway (as some WASI marketing says things like pre-open eg your code can't change the file policy after instantiation).

Another way is to allow a filter, which would be easier. Ex, if downstream are a subset of the initial files, that's safer to write.

Impl notes: internals are a bit awkward to re-expose as an FS due to how they are deconstructed during initialization, and that files can be currently open by the time you get to a host function call. This will get a little more interesting when we support an interface that can open files. https://github.com/tetratelabs/wazero/blob/ab25a84b4f4894384a78fc92560ae96dedd96122/internal/integration_test/fs/fs_test.go

@knqyf263
Copy link
Contributor Author

knqyf263 commented May 16, 2022

oh.. (sorry jet lag) @knqyf263 are you also asking to have host functions read the FS? Maybe you can clarify what you need from this below:

  1. have host functions read the current FS (currently only wasm can)
  2. override the current FS for downstream calls (currently only via module instantiation)
  • note we have both root and working directory FS at the moment

I just need only No. 2 for now.

@codefromthecrypt
Copy link
Contributor

ok I will make an experimental FSKey (not WorkDirFSKey), and refer to the godoc on ModuleConfig.WithFS

This will shadow any existing open files and whatnot.. I suspect we'll need to return a Close function of sorts, so that it can be cleaned up after the function call (similar to how a CancelContext works)

@codefromthecrypt
Copy link
Contributor

well it might not be me personally, I might try to talk @anuraaga into doing this as I'm readying a talk for Gluecon :D

@knqyf263
Copy link
Contributor Author

If you are busy, I can work on it if you guide me.

@knqyf263
Copy link
Contributor Author

After taking a quick look, I guess this fs.FS should be replaced with the new fs.FS passed through context.Context.

entry, errno := openFileEntry(dir.FS, path.Join(dir.Path, string(b)))

And I'm considering how we should populate openedFiles.

@codefromthecrypt
Copy link
Contributor

that's great!

So SysContext has a few FS related functions in it. I think this needs to be extracted into a new type including a closer. Ex FsContext or something, but the name isn't terribly important as it is internal anyway. SysContext.Close should dispatch to SysContext.FS Close

Ex. add FsContext to internal/wasm/fs.go which extracts the FS related functions of SysContext including its necessary new and Close parts.

Then, we need to change the wasi impl to use that type instead of SysContext directly.

Ex. instead of

	sys := sysCtx(m)

	if ok, err := sys.CloseFile(fd); err != nil {
	fs := fsCtx(m)

	if ok, err := fs.CloseFile(fd); err != nil {

The next step would be then making an overlay using the context. there a few parts to this:

  1. Make an internal/experimental/fs.go with FSKey inside
  • I think this needs to be an internal package to ensure no circular deps
  1. Add a function to /experimental/experimental.go like WithFS(parent context.Context, fs.FS) (ctx context.Context, close api.Closer
  • This uses similar logic to how module config made the SysContext, except only the FsContext is allocated, and the closer dispatches to FsContext.Close
  1. Change wasi to consider the ctx when getting the fsCtx
  • Ex. fs := fsCtx(m) -> fs := fsCtx(m, ctx)
  1. add an example test in experimental/fs_test.go similar to sys_test.go

Of course unit tests, but docs really only needed on the public type. Hope this sketch helps

@knqyf263
Copy link
Contributor Author

knqyf263 commented May 17, 2022

@codefromthecrypt I opened a draft PR. I will add tests, but before that, I want to make sure I'm in the right direction. Could you take a quick look when you have time?
#571

mathetake pushed a commit that referenced this issue May 20, 2022
Fixes #563 

Signed-off-by: knqyf263 <[email protected]>
Co-authored-by: Adrian Cole <[email protected]>
@codefromthecrypt
Copy link
Contributor

#922 will remove this feature because it was only used once. @mathetake found another way to do it here aquasecurity/trivy#3299

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants