-
Notifications
You must be signed in to change notification settings - Fork 19
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
dirent::d_ino #65
Comments
The ability to compare two files' inode numbers is a common way to test whether two file descriptors refer to the same file. If we make On what platforms is computing |
eg. nuttx doesn't have it in struct dirent. eg. littlefs doesn't seem to have handy on-disk numbers appropriate for the purpose. |
Ok, so then it seems to question is: should we make wasi-filesystem less useful for many applications, in order to make it more useful for people that want to use it on nuttx and littlefs? |
my suggestion is merely to define an official way for apps to know the lack of the certain underlying feature. (file number) |
Comparing inode numbers is a common Unix idiom. We can define a new API or a new sentinel convention, but application code ported to WASI from other platforms wouldn't know to use it. Nuttx advertises itself as POSIX compatible, but in the code it has this comment. What should we do? |
sure, but it won't make the situation any worse. some more data points:
|
The WASI Subgroup recently started work on an official testsuite. Once this is ready, we can add a test ensuring that two different files have differing inode numbers. Then, any implementation that wants to pass the testsuite will need to implement a d_ino field. Windows' |
where can i find it?
does it mean three calls per entry? ie. get the handle, GetFileInformationByHandle, close the handle. in any case, i'm sure that the readdir usage without checking d_ino is far more common than the programming idiom. |
The repository is at https://github.com/WebAssembly/wasi-testsuite/. As I said, we recently started work on it so there are no tests there yet, but this is the path forward.
There are users that use Here's an idea: What if we said it's ok for That way, wasi-libc users would continue to get a working |
thank you for the link.
while it would work for platforms which doesn't provide d_ino equivalent, probably |
… value. On some systems, `fd_readdir` may not implement the `d_ino` field and may set it to zero. When this happens, have wasi-libc call `fstatat` to get the inode number. See the discussion in WebAssembly/wasi-filesystem#65 for details.
I've now posted WebAssembly/wasi-libc#345 to implement the wasi-libc change discussed here. |
… value. On some systems, `fd_readdir` may not implement the `d_ino` field and may set it to zero. When this happens, have wasi-libc call `fstatat` to get the inode number. See the discussion in WebAssembly/wasi-filesystem#65 for details.
… value. On some systems, `fd_readdir` may not implement the `d_ino` field and may set it to zero. When this happens, have wasi-libc call `fstatat` to get the inode number. See the discussion in WebAssembly/wasi-filesystem#65 for details.
`readdir` returning bytes has been a source of complexity for both libc and engine implementors, with subtle issues about alignment and partial records. Rename `dirent` to `dir-entry` for readability, and revamp it: - Make the name field a proper `string` rather than being represented by trailing bytes in the buffer. - Make the inode field `optional`, so that we can make it optional as discussed in #65 without special-casing zero, which is reportedly a valid inode number on [some filesystems]. And remove the `rewind` parameter, which isn't needed when we return a stream, as users wanting to start at the beginning can just request a new stream. [some filesystems]: https://sourceware.org/pipermail/libc-alpha/2022-September/142059.html
`readdir` returning bytes has been a source of complexity for both libc and engine implementors, with subtle issues about alignment and partial records. Rename `dirent` to `dir-entry` for readability, and revamp it: - Make the name field a proper `string` rather than being represented by trailing bytes in the buffer. - Make the inode field `optional`, so that we can make it optional as discussed in #65 without special-casing zero, which is reportedly a valid inode number on [some filesystems]. And remove the `rewind` parameter, which isn't needed when we return a stream, as users wanting to start at the beginning can just request a new stream. [some filesystems]: https://sourceware.org/pipermail/libc-alpha/2022-September/142059.html
Unlike Other than that, I think the basic design you described here is what I've proposed; |
… value. (#345) * If `fd_readdir` returns a zero inode, call `fstatat` to get the inode value. On some systems, `fd_readdir` may not implement the `d_ino` field and may set it to zero. When this happens, have wasi-libc call `fstatat` to get the inode number. See the discussion in WebAssembly/wasi-filesystem#65 for details. * Update the `d_type` field too, in case it changes.
`readdir` returning bytes has been a source of complexity for both libc and engine implementors, with subtle issues about alignment and partial records. Rename `dirent` to `dir-entry` for readability, and revamp it: - Make the name field a proper `string` rather than being represented by trailing bytes in the buffer. - Make the inode field `optional`, so that we can make it optional as discussed in #65 without special-casing zero, which is reportedly a valid inode number on [some filesystems]. And remove the `rewind` parameter, which isn't needed when we return a stream, as users wanting to start at the beginning can just request a new stream. [some filesystems]: https://sourceware.org/pipermail/libc-alpha/2022-September/142059.html
It feels like this makes readdir needlessly expensive. For example, in Wazero we use the Go FS for our WASI FS implementation, meaning that it's a flexible FS backend for our users. Most real FS backends provide us with inodes, so we can pass that into WASI, but memory and virtual FS systems do not have inodes. It would be nice if we could set d_ino to something to say that the FS is never going to return an inode, and that it then not calls the fstatat (maybe -1?) There's also a bit of a flaw in the reasoning behind adding this, yes, inode is commonly used to check if a file is the same. But this should always happens in combination with st_dev, because st_ino is not unique across multiple devices. Howerver, WASI does not provide for st_dev. Should we somehow combine st_dev and st_ino? |
The resolution here should make readdir at the WASI level less expensive on these kinds of filesystems, while making readdir at the C/POSIX abstraction level approximately the same cost as it was before. If you're saying that we shouldn't pay the cost at the C/POSIX abstraction level, then I'd ask the same question I asked above: is efficiency at the C/POSIX level on these specialized filesystems worth teaching all existing application code written at the C/POSIX abstraction level that
WASI does provide for |
Not saying that, just looking for a solution where we do not have to do a
Huh, indeed. I was rather talking about I'm mostly feeling that wasi-libc has to assume that the runtime will fill in d_ino when it has the inode, putting in the |
That raises a different question than this issue started with. POSIX says "The st_ino and st_dev fields taken together uniquely identify the file within the system." citation. Consequently, it's impossible to implement POSIX on an implementation where Should we tell all programs that they can't rely on this POSIX guarantee? Or shall we consider those implementations that do that non-conforming?
What people do is read the
The problem is, people writing programs at the POSIX level won't be aware of this "zero means unavailable" behavior. POSIX-abiding code that sees two open files with the same inode number will assume they're the same, and misbehave. Do we just say "sorry applications, we know POSIX said you could do that, but we can't have you doing that; fix your code", or do we say "sorry engine implementors, you can't use filesystems where |
I see that as the problem of the implementation indeed, not wasi-libc's. If documentation behavior is that d_ino is optional and that compilers should do
Make sense, thanks!
I think the latter indeed. If WASI says that d_ino/ino/dev is required, then it's completely the problem of the runtime to make sure it's filled, if the FS in the runtime doesn't support it then it should probably not be used for WASI. I don't think wasi-libc should implement some shady workaround to make that work in some cases. In my opinion @yamt should just do an extra stat in their |
@jerbob92 I'm not familiar with Go FS, but I talked to someone who knows more about it, and they suggested that for some implementations of io/fs.FS (like the posix-ish os.dirFS, but not embed.FS), you can extract an inode by calling the io/fs.FileInfo.Sys() method, fallibly type-asserting the returned interface to *syscall.Stat_t, and readig its Ino field. Do you think that would work in Wazero? |
some files are virtual, ex backed by byte buffers and whatnot. others indeed use embed.FS, so we can only use *syscall.Stat_t on real files. As wasm is supposed to be virtualization with security properties etc, it would seem typical that files are not always going to be real files, and even if they were you wouldn't necessarily want to leak the real inodes. |
my 2p is that this is both an expensive requirement, and not a very portable one wrt how files are supplied in wasm. It |
fyi in benchmarks, using real files measures an order of magnitude slower for a small directory listing vs embedded FS. We'll implement what's needed, defensively, but generally speaking the more things that bind very strictly, the harder the wasi+wasm promise of portability will be to keep. One way to at least know what will break without very experienced people guessing is more tripwire tests in the wasi-libc repo or at least the webassembly org. Making a call that helps compiler portability but hurts runtime in the process may be still the path to take, but at least the context will be known more before making a decision and releasing it. my 2p |
The virtualization use case is an interesting point here. The dev+ino scheme creates two problems here: The device number part assumes that devices exist in a global namespace, which conflicts with the modularity goals of the component model. And the inode part assumes that filesystems have inodes. It's theoretically possible to implement inodes on top of a filesystem that doesn't have them, by maintaining a hash table at runtime of seen files, but that adds a fair amount of complexity and doesn't work if the filesystem supports renaming. So what if we go the other direction, and remove device and inode numbers? This would mean the POSIX-required And as an added bonus, this would hide more implementation details that might be leaked in the device and inode numbers. So this might make sense. But it will mean that codebases that use Thoughts? |
As discussed [here], remove the fields which correspond to `st_dev`, `st_ino`, and `d_ino` in POSIX from the stat and directory entry structs. - Device numbers assume the existence of a global device number space, which creates implicit relationships between otherwise unrelated components. - Not all filesystem implementations have these numbers. And some that do have these numbers require extra implementation cost to retrieve them. - These numbers leak potentially sensitive or identifying information from the underlying filesystem implementation. In their place, provide two functions, `is-same-file` and `is-same-mountpoint`, for explicitly testing whether two handles are the same file or are on the same mountpoint, respectively. This doesn't cover all possible use cases for device and inode numbers, but we can add more functions as need arises. [here]: #65 (comment)
I've now submitted #81 to propose removing the device and inode numbers. |
As discussed [here], remove the fields which correspond to `st_dev`, `st_ino`, and `d_ino` in POSIX from the stat and directory entry structs. - Device numbers assume the existence of a global device number space, which creates implicit relationships between otherwise unrelated components. - Not all filesystem implementations have these numbers. And some that do have these numbers require extra implementation cost to retrieve them. - These numbers leak potentially sensitive or identifying information from the underlying filesystem implementation. In their place, provide two functions, `is-same-file` and `is-same-mountpoint`, for explicitly testing whether two handles are the same file or are on the same mountpoint, respectively. This doesn't cover all possible use cases for device and inode numbers, but we can add more functions as need arises. [here]: #65 (comment)
… value. (WebAssembly#345) * If `fd_readdir` returns a zero inode, call `fstatat` to get the inode value. On some systems, `fd_readdir` may not implement the `d_ino` field and may set it to zero. When this happens, have wasi-libc call `fstatat` to get the inode number. See the discussion in WebAssembly/wasi-filesystem#65 for details. * Update the `d_type` field too, in case it changes.
As discussed [here], remove the fields which correspond to `st_dev`, `st_ino`, and `d_ino` in POSIX from the stat and directory entry structs. - Device numbers assume the existence of a global device number space, which creates implicit relationships between otherwise unrelated components. - Not all filesystem implementations have these numbers. And some that do have these numbers require extra implementation cost to retrieve them. - These numbers leak potentially sensitive or identifying information from the underlying filesystem implementation. In their place, provide some functions, `is-same-object`, `metadata-hash`, and `metadata-hash-at`, for explicitly testing whether two handles are the same file or have the same metadata, respectively. This doesn't cover all possible use cases for device and inode numbers, but we can add more functions as need arises. [here]: #65 (comment)
As discussed [here], remove the fields which correspond to `st_dev`, `st_ino`, and `d_ino` in POSIX from the stat and directory entry structs. - Device numbers assume the existence of a global device number space, which creates implicit relationships between otherwise unrelated components. - Not all filesystem implementations have these numbers. And some that do have these numbers require extra implementation cost to retrieve them. - These numbers leak potentially sensitive or identifying information from the underlying filesystem implementation. In their place, provide some functions, `is-same-object`, `metadata-hash`, and `metadata-hash-at`, for explicitly testing whether two handles are the same file or have the same metadata, respectively. This doesn't cover all possible use cases for device and inode numbers, but we can add more functions as need arises. [here]: WebAssembly#65 (comment)
As discussed [here], remove the fields which correspond to `st_dev`, `st_ino`, and `d_ino` in POSIX from the stat and directory entry structs. - Device numbers assume the existence of a global device number space, which creates implicit relationships between otherwise unrelated components. - Not all filesystem implementations have these numbers. And some that do have these numbers require extra implementation cost to retrieve them. - These numbers leak potentially sensitive or identifying information from the underlying filesystem implementation. In their place, provide some functions, `is-same-object`, `metadata-hash`, and `metadata-hash-at`, for explicitly testing whether two handles are the same file or have the same metadata, respectively. This doesn't cover all possible use cases for device and inode numbers, but we can add more functions as need arises. [here]: WebAssembly#65 (comment)
* Remove the device and inode numbers from the API. As discussed [here], remove the fields which correspond to `st_dev`, `st_ino`, and `d_ino` in POSIX from the stat and directory entry structs. - Device numbers assume the existence of a global device number space, which creates implicit relationships between otherwise unrelated components. - Not all filesystem implementations have these numbers. And some that do have these numbers require extra implementation cost to retrieve them. - These numbers leak potentially sensitive or identifying information from the underlying filesystem implementation. In their place, provide some functions, `is-same-object`, `metadata-hash`, and `metadata-hash-at`, for explicitly testing whether two handles are the same file or have the same metadata, respectively. This doesn't cover all possible use cases for device and inode numbers, but we can add more functions as need arises. [here]: #65 (comment) * Remove the device and inode numbers from the API. As discussed [here], remove the fields which correspond to `st_dev`, `st_ino`, and `d_ino` in POSIX from the stat and directory entry structs. - Device numbers assume the existence of a global device number space, which creates implicit relationships between otherwise unrelated components. - Not all filesystem implementations have these numbers. And some that do have these numbers require extra implementation cost to retrieve them. - These numbers leak potentially sensitive or identifying information from the underlying filesystem implementation. In their place, provide some functions, `is-same-object`, `metadata-hash`, and `metadata-hash-at`, for explicitly testing whether two handles are the same file or have the same metadata, respectively. This doesn't cover all possible use cases for device and inode numbers, but we can add more functions as need arises. [here]: #65 (comment) * Explicitly document that the hash can contain a secret value. * Use a named `record` type instead of a `tuple` for hash values.
depends on the underlying platforms and/or filesystems, it can be very expensive to provide dirent::d_ino.
i'd suggest to make it optional.
eg. allow a wasi implementation to say it unknown in a way similar to filetype.unknown.
how do you think?
cf. https://github.com/bytecodealliance/wasm-micro-runtime/blob/b49fb8a66840d484e775936399456fb6b85f98c6/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c#L2101-L2105
The text was updated successfully, but these errors were encountered: