feat: Limit the range of randomly generated fd in the WASI component#4549
feat: Limit the range of randomly generated fd in the WASI component#4549uddhavphatak wants to merge 2 commits intoWasmEdge:masterfrom
Conversation
github-actions
bot
added
c-CLI
|
This is my attempt at trying to solve this issue, please review and tell if I can improve anything. If everything is fine, I will write this into the documentation |
include/driver/tool.h
Outdated
| .add_option("enable-component"sv, PropComponent) | ||
| .add_option("enable-all"sv, PropAll) | ||
| .add_option("time-limit"sv, TimeLim) | ||
| .add_option("max-fd"sv, MaxWasiFd) |
There was a problem hiding this comment.
@LFsWang Fixed here, also spotted this issue in other file declarations, mainly due to my habitual use of tab instead of spaces. Thus fixed at those places as well
uddhavphatak
force-pushed
the
master
branch
2 times, most recently
from
ee942ec to
8b069cb
Compare
uddhavphatak
force-pushed
the
master
branch
2 times, most recently
from
979abd0 to
6f14c99
Compare
hydai
left a comment
hydai
left a comment
There was a problem hiding this comment.
This PR doesn't contain any basic verifications and exhibits some unusual behaviors, including:
- What happens if users set the maximum WASI FD to 0?
- Hard-coding 0x7FFFFFF in the C API.
- I don't think we need to change the
initseries functions. ThegenerateRandomFdToNodeis not used in these functions. Instead, we should set the MaxFD in another phase.
github-project-automation
bot
moved this from Triage-required
to In progress
in WasmEdge Roadmap
|
Converted to a draft PR since the contributor is not running CI on their own forked repo. There are some very basic CI checks that must fail. It's not necessary to approve the CI to avoid wasting our resources on this PR. |
Signed-off-by: uddhav Phatak <[email protected]>
github-actions
bot
removed
c-CAPI
|
@hydai, It was mistake from my end, I had understood the issue wrong, thus thought that I had to make changes in the init API. I also have setup the CI on my fork, and done the CI testing before reopening the issue. should I undraft this PR? |
include/common/configure.h
Outdated
| std::atomic<bool> CoredumpWasmgdb = false; | ||
| std::atomic<bool> ForceInterpreter = false; | ||
| std::atomic<bool> AllowAFUNIX = false; | ||
| uint32_t MaxWasiFd = 0x7FFFFFFF; |
include/host/wasi/environ.h
Outdated
| return std::string(Buffer.data(), Buffer.size()); | ||
| } | ||
|
|
||
| void setMaxWasiFd(uint32_t Limit) noexcept { |
There was a problem hiding this comment.
Your naming is inconsistent. Some of them use FD, while others use Limit.
include/host/wasi/environ.h
Outdated
| } | ||
|
|
||
| void setMaxWasiFd(uint32_t Limit) noexcept { | ||
| MaxFd = std::clamp(Limit, 3U, 0x7FFFFFFFU); |
There was a problem hiding this comment.
You are silently modifying the user's given limit; however, this is not mentioned in the option description.
Signed-off-by: uddhav Phatak <[email protected]>
| WasiExpect<__wasi_fd_t> generateRandomFdToNode(std::shared_ptr<VINode> Node) { | ||
| std::uniform_int_distribution<__wasi_fd_t> Distribution(0, 0x7FFFFFFF); | ||
| std::uniform_int_distribution<__wasi_fd_t> Distribution( | ||
| 0, static_cast<__wasi_fd_t>(MaxFd)); |
There was a problem hiding this comment.
There are many possible cases that are not covered.
For example, what if MaxFd = 3, but there are more than one preopened folder?
What if MaxFd is fully used, but we are trying to open a new one? The original limitation is large enough to prevent such a case. However, your new approach may not work.
There was a problem hiding this comment.
@hydai I see the issue, but I have a few doubts.
can the user set the MaxFd as 3?, as it is a CLI based command (sets max limit when option "max-fd" is used), but by default, the limitation is large enough.
and in the case if MaxFd is fully used, but we are trying to open a new one. should we return an error? or should I create an implementation that will create a queue to track the files which are open/closed? WDYT
There was a problem hiding this comment.
That's why I am considering it.
The reason we want this feature is that the issue report uses select, which accepts fewer than 1024 file descriptors. I prefer to set the minimum to 1024; if the given MaxFd is less than this, then we should clamp it to 1024. If MaxFd is fully used, we should return an error directly. I don't think having a queue for this is a good idea, since execution may rely on this new FD to perform certain tasks.
3 participants
Motivation
Due to some applications using the old select API, our current implementation follows the standard of WASI which means we will have a map to handle the randomly generated fd number (
<= 2**31) to the actual fd number. However, in some cases, the application only accepts the fd number which is less thanFD_SETSIZE (usually 1024).It would be nice to have an API to set the range of the randomly generated fd numbers.
Details
solves issue #2102