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

[WASI] Check fd valid on socket_accept, fix #3041 #3043

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

ibmibmibm
Copy link
Collaborator

No description provided.

Signed-off-by: Shen-Ta Hsieh <beststeve@secondstate.io>
Copy link
Member

juntao commented Nov 20, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Commit 17d8e9bbcd82aa7c260a1eadfd336bd1057ecec1

The commit fixes issue #3041 by adding a check to verify if the file descriptor (fd) is valid in socket_accept in the WASI Environ class.

Here's a summary of changes in the patch:

  1. In the Environ class, before the socket_accept function is called to accept a new socket, the node associated with the file descriptor (Fd) is fetched using getNodeOrNull.
  2. The patch adds a check if the Node is nullptr (implying that the file descriptor is invalid). If it is, it returns an error __WASI_ERRNO_BADF.
  3. If the Node is not null (file descriptor is valid), then it proceeds to call socket_accept. The existing code checked for an error in socket_accept and this check still remains.

Potential Problems:

  1. There's no check before getNodeOrNull call. Fd should be checked to see if it is in the proper range or valid before being passed to getNodeOrNull. In other words, this patch does not consider the scenario where the file descriptor itself could be invalid.
  2. This check potentially introduces a change in function behavior that could impact other parts of the code base where the function is used. Therefore, the change requires careful testing to ensure that it does not create any new issues.
  3. This patch also does not address any potential thread safety or concurrency issues, i.e., what happens if another operation invalidates the node after the check but before the call to socket_accept.
  4. Finally, the error handling of socket_accept remains unchanged. It's still returning the result of socket_accept directly which doesn't seem proper. It should wrap the error with __WASI_ERRNO_BADF, similar to the new fd invalid case handling, otherwise the outcome could be misleading in cases where socket_accept fails.

@ibmibmibm ibmibmibm requested a review from hydai November 20, 2023 09:25
@github-actions github-actions bot added the WASI https://github.com/WebAssembly/WASI label Nov 20, 2023
@ibmibmibm ibmibmibm marked this pull request as ready for review November 20, 2023 09:33
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (9ba5f8f) 80.70% compared to head (17d8e9b) 80.68%.

Files Patch % Lines
include/host/wasi/environ.h 0.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3043      +/-   ##
==========================================
- Coverage   80.70%   80.68%   -0.02%     
==========================================
  Files         151      151              
  Lines       21666    21668       +2     
  Branches     4393     4394       +1     
==========================================
- Hits        17485    17483       -2     
- Misses       3003     3004       +1     
- Partials     1178     1181       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ibmibmibm ibmibmibm merged commit 60773e5 into master Nov 20, 2023
37 of 40 checks passed
@ibmibmibm ibmibmibm deleted the beststeve/wasi-socket-accept branch November 20, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WASI https://github.com/WebAssembly/WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants