Refactor/sdk preview

[L-jasmine]

Refactor the SDK to resolve memory-unsafe bugs caused by its usage.

See https://github.com/WasmEdge/wasmedge-rust-sdk/pull/83

[juntao]

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

---

Overall Summary:

In general, the pull request titled "Refactor/sdk preview" includes a variety of changes across different files. The change sets include adding attributes, fixing memory leaks, removing unused code, adding new files and functionality, modifying existing code, and making adjustments to dependencies.

Important Findings:

1. There are several potential issues and errors that need to be addressed, such as typos, misuse of attributes, unhandled error cases, and unclear changes without proper context or explanation.

2. There are significant refactorings throughout the codebase. It is important to carefully review these changes to ensure correctness and avoid introducing new bugs.

3. Some changes result in the introduction of new functionality, like managing socket writability and handling socket events, implementing a virtual file system, and adding timeout functionality. However, there are potential problems and lack of documentation to understand these changes fully.

4. The commit messages and pull request descriptions often lack context and explanation, making it difficult to understand the purpose and motivation behind the changes. It is recommended to provide more detailed information in the commit messages or pull request description.

Overall, the pull request shows a mix of potential issues, important findings, and new functionality. Addressing the potential problems and providing more documentation will be crucial for a comprehensive review.

Details

Commit ecaa6cb9da0f3a25d771ce653e61350522966252

Key changes:

• Added #[form] attribute to the Core, Store, Vm, Func, Mem, Global, Table, Import, Export, Instance, and Plugin variants of the WasmEdgeError enum.

Potential problems:

• There is a typo in the commit message: Add #[form] for subtypes of WasmEdgeError. It should be Add #[from] for subtypes of WasmEdgeError.
• The #[form] attribute should be #[from].
• The commit message does not provide enough context or explanation for the changes. It would be helpful to include a more detailed description of why the changes are being made.

Commit 2116d0b7125504d8b3323365ff41cf3e85761243

The key changes in this patch include:

• Fixing memory leaks in the codebase.
• Refactoring the codebase by removing unnecessary imports and unused code.
• Adding new code files related to async functionality.

One potential problem is the deletion of the Drop implementation for ExportType and Module. This may cause memory leaks if the inner field is not properly freed.

Another potential problem is the removal of code related to Externals, including functions, globals, memory, and tables. This may impact the functionality of the codebase if these features were being used.

It is also worth noting that there are changes in a large number of files, indicating significant refactorings throughout the codebase. It would be important to review these changes in detail to ensure they are correct and do not introduce new bugs.

Commit 949c9876187cfc1a7105452b1792cb06acf8a7f9

Key changes in the patch:

1. Added a new struct SocketWritable to handle checking and setting socket writability.
2. Added a new struct SocketWritableFuture as a future for checking socket writability.
3. Added a writable() method to SocketWritable to asynchronously wait for the socket to become writable.
4. Added a writable() method to AsyncWasiSocket to set the socket as writable and await for it to become writable.
5. Modified the readable() method in AsyncWasiSocket to use the readable() method from AsyncWasiSocketInner.
6. Modified the readable() method in AsyncWasiSocket to return Ok(()) instead of Ok(None).
7. Modified the readable() method in AsyncWasiSocket to clear the ready state of the socket after reading.
8. Added a new field writable of type SocketWritable to AsyncWasiSocket.

Potential problems:

1. In the readable() method of AsyncWasiSocket, the match expression on r does not handle the case when r is Err(e).
2. In the wait_fd() function, the handler closure does not properly handle the case when the Result is an error. The current implementation sets the error field of __wasi_event_t but does not handle it in the return value.
3. The AsyncWasiSocket struct now has a new field writable which is not used in the existing code. It is unclear how this field is intended to be used.
4. The changes in AsyncWasiSocket::readable() to clear the ready state of the socket after reading may have unintended consequences if the clearing of ready state is not necessary or if it is done at the wrong time.

Overall, the changes seem to introduce new functionality for managing socket writability and handle socket events. However, there are potential problems that need to be addressed and more information is needed to understand the use case for the new writable field in AsyncWasiSocket.

Commit 0524f2267559f5dc6517f5329078a1fd1a51f961

Key changes:

1. Added the line socket.writable.set_writable(); to the code.

Potential problems:

1. There is no description or explanation provided for the purpose of the change. It would be helpful to have more context to understand why this line was added.

Overall, the changes seem simple and isolated to one file in the project. However, without further information, it is difficult to assess the impact of this change and whether it is necessary or if there might be any unintended side effects. It is recommended to ask the author for more details about the change.

Commit d7f841eda90b63b9beab06d3178bd10282f42b3d

Key Changes:

• The code has added two lines in the args_get function and two lines in the environ_get function.
• Both additions set a null terminator after copying a slice into a buffer.

Potential Problems:

• There seems to be a potential off-by-one error in both additions. The null terminator is set after copying the slice, but the len argument should be the length of the copied slice plus one for the null terminator. This might cause incorrect behavior or buffer overflows.
• The code does not include any comments explaining the purpose or rationale behind the changes. It would be helpful to have some context to understand the intention behind these modifications.

Commit 7408e6278d4172c303bc214adc736b8b0c270d63

Key changes in this GitHub patch:

1. In the file async_tokio.rs, the state field of the AsyncWasiSocket struct has been changed from WasiSocketState to Box<WasiSocketState>.
2. In the file sync.rs, the seek method of the WasiFile struct has been refactored to set the seek position before reading or writing to the file.
3. In the file mod.rs, the WasiCtx struct was modified. The vfs field was changed from an Arc<RwLock<ObjectPool<VFD>>> to an ObjectPool<VFD>. The push_preopen, insert_vfd, remove_closed, remove, get, and get_mut methods were modified to use the new vfs field directly instead of the RwLock. The vfs field initialization was also modified to directly assign the vfs parameter value.

Potential problems:

1. It's unclear why the state field in AsyncWasiSocket is changed to a Box<WasiSocketState>. This change could have impacts on other parts of the codebase that rely on this field being directly accessible without indirection.
2. The refactoring in sync.rs could introduce bugs if the seek position is not correctly set before reading or writing to the file.
3. The modification of the WasiCtx struct and related methods could introduce synchronization issues since the Arc<RwLock<ObjectPool<VFD>>> has been replaced with a direct ObjectPool<VFD>. This change could impact the thread-safety of the code if multiple threads access the vfs field simultaneously.

Commit 882a3b2cd00003615b06105965233fd44a4dd98b

Key changes:

• The serialize feature has been temporarily disabled.
• The dependencies have been formatted with spaces between the keys and values.

Potential problems:

• It is not clear why the serialize feature has been disabled. This change should be documented in the commit message or pull request description.
• The formatting of the dependencies could be improved for consistency. Some dependencies have spaces before the equal sign and others do not.
• The dev-dependency serde_json has not been formatted consistently with spaces between the keys and values.
• The serialize feature has been removed from the default feature set, but it is not clear if this is intentional or accidental. This change should also be clarified in the commit message or pull request description.

Commit 47a360488a27dcc65f759517938b9556c81b59ad

Key changes:

• The patch adds a new feature called "virtual file system".
• The WasiStdin struct has been modified to store a boxed trait object (Read + Send) instead of a plain struct. The Debug trait implementation has also been changed.
• The WasiStdout struct has been modified similarly to WasiStdin.
• Implementations for various methods have been added to both WasiStdin and WasiStdout.

Potential problems:

• It's unclear what the purpose of the "virtual file system" feature is and how it's intended to be used. The changes made to WasiStdin and WasiStdout introduce new traits (WasiVirtualFile and WasiVirtualNode) and methods, but there is no explanation or documentation provided for what these traits represent or how they are intended to be used. It would be helpful to add some comments or documentation to clarify their purpose.
• The changes made to WasiStdin and WasiStdout seem to be incomplete. Some methods have been implemented, but others (like fd_read in WasiStdout and fd_tell and fd_seek in WasiStdin) are stubbed out and return error codes. It's important to ensure that all required methods are implemented correctly and consistently.
• There are no tests provided for the new feature. It's important to thoroughly test any new functionality that is introduced. It would be helpful to add some test cases to verify the correctness of the WasiVirtualFile and WasiVirtualNode implementations and the behavior of the WasiStdin and WasiStdout structs.
• The commit message is vague and does not provide enough information about the purpose or motivation behind the changes. It would be helpful to provide more context and explanation in the commit message to give reviewers a better understanding of the changes being made.

Commit f71e855d8372776f38c758819d9642cdcba18560

Key changes in the patch:

• The argument types of run_func_with_timeout and run_func_async_with_timeout functions have been updated.
• timeout_sec argument has been replaced with deadline argument of type std::time::SystemTime.
• The timeout argument in call_func_with_timeout function has been changed to std::time::Duration.

Potential problems:

• It seems that the changes made are related to implementing a timeout for function execution. However, it is not clear how the timeout functionality is implemented in the code.
• The code changes have been cherry picked from a commit, but the details of the original commit are not mentioned, which makes it difficult to understand the context of the changes.
• There are no comments or explanations provided along with the code changes, which could make it difficult for future developers to understand the purpose and functionality of the code. It would be helpful to add comments or documentation explaining the changes made.

Commit 317c7a990f8fc0e781df02334b18fd23b0f91ba8

Key changes:

• The registered field in the Validator struct has been removed.
• The registered field is no longer checked in the Drop implementation.

Potential problems:

• It seems that the removal of the registered field may have unintended consequences. It is possible that the registered field was used somewhere else in the codebase, and its removal could lead to unexpected behavior or bugs. It would be good to review the codebase and ensure that there are no dependencies on this field before merging this patch.
• The removal of the check for registered in the Drop implementation could result in a double deletion of the inner resource if the validator is not registered but the inner resource is non-null. This could lead to memory corruption or crashes. It's important to ensure that the inner resource is properly managed and that this change does not introduce any memory management issues.
• Since the patch only modifies a single file, potential issues related to other files or dependencies cannot be determined from this patch alone. A comprehensive review of the entire codebase is recommended to ensure that these changes do not introduce new problems or issues in other parts of the code.

Commit 89ea9a2713dd3da31b18cfa9fde172e1a7f6d58e

Key changes in the patch:

• Refactoring of the code to pass Clippy linter warnings and improve code quality.
• Changes related to implementing async/await in the codebase.
• Various changes and fixes to function, module, and executor implementations.

Potential problems:

• The patch seems to introduce async/await functionality to the codebase. It would be important to ensure that the changes are correct and don't introduce bugs or impact the performance of the application.
• The code uses unsafe blocks in a few places. It is crucial to review these blocks thoroughly and make sure they don't introduce any vulnerabilities or unsafe behavior.
• The patch includes changes to multiple files across the codebase. It would be helpful to review the changes in each file in detail to ensure that they are correct and address the intended improvements.

Commit 7cfb54777a586951fa4fe879dfc8639ab94abb35

Key changes:

• The patch disables the timeout feature in musl libc for the wasmedge-sys crate.
• Several conditional compilation blocks have been added to exclude code when compiling for Linux with musl libc.

Potential problems:

• It's unclear why the timeout feature is being disabled specifically for musl libc. A comment explaining the reasoning would be helpful.
• It's recommended to add tests to ensure the code behaves as expected after the changes.

Overall, the code changes seem reasonable, but further clarification and tests would be beneficial.

Commit bf772c0d7b52818c0736e9ecd0a9da5d0701b0c3

Key Changes:

• The run_func_with_timeout method in the Vm struct has been modified.

Potential Problems:

• The patch disables the timeout in the run_func_with_timeout method for musl libc when the target OS is Linux.
• The reason for disabling the timeout is not clear in the patch description.
• It is unclear how the timeout was implemented prior to this change and the impact of disabling it.

Suggestions for Improvement:

• Provide a more detailed explanation in the patch description about why the timeout was disabled.
• If possible, provide an alternative solution for handling timeouts in musl libc.
• Consider adding comments in the code to explain the purpose and reasoning behind disabling the timeout.

Commit 2ffdbb0f96c270faf76d43e94ff2c8b3050c4c81

Key changes in the patch:

1. In crates/wasmedge-sys/src/async/module.rs, commented out the creation of an async function.
2. In crates/wasmedge-sys/src/frame.rs, added a new method memory_mut to retrieve a mutable reference to a memory instance by its index.
3. In crates/wasmedge-sys/src/store.rs, updated the documentation comment to clarify the purpose of the Store struct.
4. In src/async/import.rs and src/import.rs, updated the documentation comment to clarify the purpose of the ImportObjectBuilder struct.
5. In src/store.rs, updated the documentation comment to clarify the purpose of the Store struct.

Potential problems:

1. The comment in crates/wasmedge-sys/src/async/module.rs suggests that an async function was commented out. If this function is needed, its removal may cause functionality issues.
2. The new method memory_mut in crates/wasmedge-sys/src/frame.rs is not documented or explained further. It would be helpful to provide more context and examples for its usage.
3. The documentation comments in src/async/import.rs, src/import.rs, and src/store.rs could be more detailed and provide more examples to help users understand their purpose and usage.

Commit 9c95c51facbca52b324ce7d973f111cef0ae4a37

Key changes:

• The From<&ffi::WasmEdge_String> for String implementation in the types.rs file has been refactored.
• The previous implementation converted the &ffi::WasmEdge_String directly to &std::ffi::CStr and then used to_string_lossy() to convert it to an owned String.
• The refactored implementation now directly converts the s.Buf pointer to a raw byte slice and uses String::from_utf8() to convert it to a String.
• This change is intended to improve performance and avoid unnecessary conversions.

Potential problems:

• The new implementation uses unsafe code to directly manipulate raw pointers.
• There is a potential risk of undefined behavior if the s.Buf pointer is not valid or if the slice length does not match the actual length of the string.
• The new implementation uses String::from_utf8().unwrap_or_default() which returns an empty string if the conversion fails. It might be worth considering whether this is the desired behavior or if a different error handling approach should be used.

Overall, the refactoring seems reasonable, but it is important to carefully evaluate the safety and correctness of the unsafe code and consider error handling.

Commit aa2639d36340822e31aeae59d67df80d5b7b131f

Key changes:

• The patch includes changes to multiple files in the project.
• Some files have been significantly refactored, with a large number of lines added and deleted.
• The wrap_sync_wasi_fn and wrap_async_wasi_fn functions have been removed.
• The HostFn type has been removed.

Potential problems:

• The patch includes a large number of deletions and insertions, indicating significant changes to the codebase.
• The removal of the wrap_sync_wasi_fn and wrap_async_wasi_fn functions may impact functionality. It's important to review the changes related to these functions and ensure that any new implementations are correct.
• The removal of the HostFn type may also impact functionality. Review any changes related to the HostFn type and ensure that any new implementations are correct.
• The patch removes multiple files, including src/import.rs. Make sure the removal of this file is intentional and doesn't affect the project's functionality.
• Review the changes related to each file to ensure that there are no logical errors or unintended side-effects introduced by the refactoring.

Commit 4cccc4aef1ccc7277f85a0f802e764de7dbc92c2

Key changes in this patch include:

1. The WasiModule struct is changed from using composition to using a tuple struct with an Instance field.
2. The Drop implementation for WasiModule is removed.
3. The AsRef and AsMut trait implementations are added for WasiModule.
4. Several methods in the WasiModule implementation are modified to use the Instance field instead of the inner field.

Potential problems:

1. The WasiModule struct's Clone trait implementation is removed. It should be verified if this is intentional or if it needs to be re-implemented.
2. The Drop implementation for WasiModule is removed, which means that the underlying InnerInstance may not be properly cleaned up. This change should be reviewed to ensure that there are no memory leaks.
3. The changed WasiModule struct now exposes the Instance field directly, which could potentially lead to misuse or unintended modifications. This change should be reviewed to ensure that it doesn't introduce any issues related to encapsulation or safety.
4. The methods in the WasiModule implementation are modified to use the Instance field directly. This change should be reviewed to ensure that it doesn't introduce any logic errors or unexpected behavior.

Overall, the changes seem to be focused on refactoring the WasiModule struct and its usage. It is important to thoroughly review the changes to ensure that they don't introduce any issues or regressions.

Commit 18073d36d9f48bb515f14c691c1f29c6c55a0700

Key Changes:

• Added use crate::AsInstance; in the test_plugin_wasmedge_process and test_plugin_wasi_crypto functions.
• Replaced result.is_some() assertions with result.is_ok() assertions.
• Updated assertions for module instances to use unwrap() instead of is_some().

Potential Problems:

• The use crate::AsInstance; import may not be necessary in the test_plugin_wasmedge_process and test_plugin_wasi_crypto functions. The reviewer should verify if this import is required for the tests to function correctly.
• The assertions for the module instances rely on the unwrap() method, assuming the results will always be Some. There is no error handling for cases where the results may be None. The reviewer should consider adding appropriate error handling or validating the assumptions made in the tests.
• It's unclear from the patch if these changes are related to refactoring or fixing a specific issue. The commit message and code comments do not provide enough context. The reviewer should ask for more information or clarification on the purpose of these changes.

Commit 6132ffe6db3098e2bc11d6e0bbb6d57ec0e5399b

Key Changes:

• In the .github/workflows/ci-build.yml file, the command cargo test has been modified to include the --skip test_vmbuilder flag.

Potential Problems:

• The pull request updates the CI build workflow to skip the test_vmbuilder test on Fedora. This change might have been made for a valid reason, but it's unclear from the provided patch why this test is being skipped. It would be helpful to have some additional context or a justification for skipping this test.

Overall, this change seems to be a minor adjustment to the CI workflow.

Commit 2d27eabecfb96b840e1fde42aae30b9328139111

Key changes in the patch:

• The implementation of extracting a value from a FuncRef has been changed.
• The previous logic has been commented out and replaced with a temporary implementation using externref.

Potential problems:

• The new implementation for FuncRef extraction is not complete and is using externref instead.
• The comment suggests that it is waiting for "fun-ref related proposals to stabilize", which implies that this code might not work as intended.
• There are to-do comments left in the code, indicating incomplete implementation.

Suggestions:

• Review the intention behind the changes made for the FuncRef extraction and ensure it aligns with the project requirements.
• If the intention is to replace it temporarily with externref, make sure this temporary implementation is properly documented and communicated with other developers.
• If this change is meant to be permanent, consider completing the implementation for FuncRef extraction or removing the unused code.
• Remove the to-do comments to maintain clean and readable code.
• Consider adding tests to verify that the changes made in this patch work as expected.

Commit 17221d00a3a2446bd77431cc2e5ce74d0160a93d

The key changes in this pull request are:

• Refactoring of several files in the examples/wasmedge-sys directory
• Refactoring of the src/async/vm.rs file
• Refactoring of the src/compiler.rs file
• Refactoring of the src/executor.rs file
• Refactoring of the src/module.rs file
• Refactoring of the src/vm.rs file

Potential problems that can be found in this patch are:

• The changes seem to be quite large and affect many files. It would be good to have a clear description of the changes made in each file to understand the purpose and impact of the refactoring.
• It's also unclear what the goal of the refactoring is. Is it to improve performance, readability, or some other aspect of the codebase? A clear motivation for the changes would help in understanding the context.
• There are no tests included in the patch. It would be good to ensure that the changes don't introduce any regressions or bugs by including appropriate tests.
• The commit message formatting could be improved to provide more context about the changes made and why they are necessary.
• It appears that the same changes are made in multiple files. This may indicate a need for a more systematic approach to the refactoring, such as extracting common functionality or creating reusable components.

Overall, a clearer explanation of the changes, their purpose, and their impact would be beneficial for a thorough review of this pull request.