WASI preopen_dir holds on to Windows resource

[thewtex]

The preopen_dir WASI support seems to hold onto Windows directory resources, which prevents removal of the directory, etc.

This is demonstrated by #131 , which will result in:

C:\Users\matth\mambaforge\envs\itk-wasm\lib\tempfile.py:843: PermissionError

During handling of the above exception, another exception occurred:

path = 'C:\\Users\\matth\\AppData\\Local\\Temp\\tmp2w27_daj'
onerror = <function TemporaryDirectory._rmtree.<locals>.onerror at 0x0000024D29A84EE0>

    def _rmtree_unsafe(path, onerror):
        try:
            with os.scandir(path) as scandir_it:
                entries = list(scandir_it)
        except OSError:
            onerror(os.scandir, path, sys.exc_info())
            entries = []
        for entry in entries:
            fullname = entry.path
            if _rmtree_isdir(entry):
                try:
                    if entry.is_symlink():
                        # This can only happen if someone replaces
                        # a directory with a symlink after the call to
                        # os.scandir or entry.is_dir above.
                        raise OSError("Cannot call rmtree on a symbolic link")
                except OSError:
                    onerror(os.path.islink, fullname, sys.exc_info())
                    continue
                _rmtree_unsafe(fullname, onerror)
            else:
                try:
                    os.unlink(fullname)
                except OSError:
                    onerror(os.unlink, fullname, sys.exc_info())
        try:
>           os.rmdir(path)

[thewtex]

This came up in a migration from wasmer to wasmtime: InsightSoftwareConsortium/ITK-Wasm#773

[alexcrichton]

To some extent this is a difference between how Unix and Windows work. In some cases though, and perhaps this one, it's possible to paper over the differences. My guess is that the WASI support for preopening files holds open handles and the open handles are preventing deletion. This can probably be fixed by either:

1. Ensure that the WASI object/store/etc related to Wasmtime are all destroyed in the embedding (I don't think this is necessarily easy).
2. There may be flags to open directories with which allow concurrent deletion. I think files have this flag and this may need to be a patch to Wasmtime's WASI support.

[sunfishcode]

Is the issue here that the embedding is trying to remove the temporary directory before destroying the WasiCtx which is holding a handle to the temporary directory?

[thewtex]

There may be flags to open directories with which allow concurrent deletion. I think files have this flag and this may need to be a patch to Wasmtime's WASI support.

Agreed, I think this would be ideal.

embedding is trying to remove the temporary directory before destroying the WasiCtx which is holding a handle to the temporary directory?

We have a situation where something outside the embedding tries to remove the directory before the WasiCtx is destroyed.

[sunfishcode]

Currently, Wasmtime deliberately sets the flags that prevent concurrent deletion, on the interpretation that preopens are granted to a WASI context in the form of handles, rather than paths. Wasmtime thinks the job of the engine is to ensure that the guest has access to what's referenced by the handles, not to other things that might occupy the same path over time if the original contents are deleted.

For example, if the directory is created by something like Python's tempfile, the generated names are not guaranteed to be collision-proof, so if something outside the embedding removes the directory before the WasiCtx is destroyed, and we allow guest code to continue to access the referent of the original path, and some future tempfile invocation happens to pick that same path, that could allow guest code to gain access to directory contents it shouldn't have access to.

I'm open to making changes here, but it's important for us to guard against this kind of unintended pathname reuse bug.

[thewtex]

@sunfishcode, ah, I see your point on the need to not provided unintended access to a directory.

I do not know how wasmtime is implemented, but is it possible to retain the preopen identifier as a handle, so we do not provide access to a "different" directory, but not set the flags to prevent deletion outside of the wasmtime engine? Or something similar?

[sunfishcode]

The main Windows APIs lack a way to do something analogous to a Unix openat, taking a base handle, so currently the implementation on Windows concatenates the base directory path with the relative path and opens the result, to emulate openat. I don't know of a way to make that robust without preventing concurrent renaming or deletion of the base.

The Windows kernel API does have a function called NtCreateFile which does have the ability to take a base handle. We've begun porting some parts of our Windows implementation to use this. However, this is subtle work, and we'll have to find ways to make every function in the WASI filesystem API use this before we could stop setting the flags to prevent concurrent deletion, and I don't yet know if that's possible.