Conversation
|
related to #6055 |
artemyarulin
force-pushed
the
tests/wasix-tests
branch
from
2fcfdfb to
6684e67
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive POSIX compatibility tests for WASIX syscalls, drawing from established test suites like LTP (Linux Test Project) and stress-ng. The implementation introduces 566 new tests (149 Rust + 417 C) covering 81 out of 135 syscalls to ensure POSIX compatibility and improve test coverage.
Changes:
- Added extensive C-based test suite for WASIX syscalls covering networking, file operations, process management, and signal handling
- Fixed multiple syscall implementations to align with POSIX behavior (error codes, validation, memory safety)
- Enhanced linker and module loading to support RTLD_GLOBAL flag and proper symbol resolution
Reviewed changes
Copilot reviewed 164 out of 182 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/c-wasi-tests/*.c | New POSIX compatibility tests for various syscalls |
| lib/wasix/src/syscalls/wasix/sock_*.rs | Fixed socket option handling and validation |
| lib/wasix/src/syscalls/wasix/proc_*.rs | Improved process signal handling and validation |
| lib/wasix/src/syscalls/wasi/path_*.rs | Enhanced path operations with better error handling |
| lib/wasix/src/syscalls/wasi/fd_*.rs | Fixed file descriptor operations and rights checking |
| lib/wasix/src/state/linker.rs | Added RTLD_GLOBAL support and function table deduplication |
| lib/virtual-fs/src/pipe.rs | Implemented proper pipe buffering with PIPE_MAX_BYTES limit |
| lib/virtual-net/src/host.rs | Added IP list and route list implementations |
| if !matches!( | ||
| opt, | ||
| Sockoption::RecvBufSize | ||
| | Sockoption::SendBufSize | ||
| | Sockoption::Ttl | ||
| | Sockoption::MulticastTtlV4 | ||
| ) { | ||
| return Ok(Err(Errno::Inval)); | ||
| } |
There was a problem hiding this comment.
The validation logic checks for valid socket options but then converts to WasiSocketOption and matches again in the closure. This creates duplicate validation. Consider consolidating the validation or removing the redundant check.
| if !ctx.data().process.signal_thread(&tid, sig) { | ||
| return Ok(Errno::Srch); | ||
| } |
There was a problem hiding this comment.
The function now returns a boolean to indicate success/failure, but the error handling immediately after could be simplified by having signal_thread return a Result<(), Errno> instead of a boolean.
| if sig_u8 > Signal::Sigsys as u8 { | ||
| return Ok(Errno::Inval); | ||
| } | ||
| let sig = Signal::try_from(sig_u8).unwrap_or(Signal::Signone); |
There was a problem hiding this comment.
The validation logic converts i32 to u8, checks bounds, then converts to Signal with unwrap_or. Consider using Signal::try_from directly on the bounded sig_u8 and handling the error case explicitly instead of using unwrap_or, as this would make the intent clearer.
| let sig = Signal::try_from(sig_u8).unwrap_or(Signal::Signone); | |
| let sig = match Signal::try_from(sig_u8) { | |
| Ok(sig) => sig, | |
| Err(_) => return Ok(Errno::Inval), | |
| }; |
| fn resolve_symlink_path( | ||
| state: &WasiState, | ||
| inodes: &WasiInodes, | ||
| path: &str, | ||
| ) -> Result<String, Errno> { | ||
| let mut seen: HashSet<PathBuf> = HashSet::new(); | ||
| let mut current = PathBuf::from(path); | ||
|
|
||
| for _ in 0..MAX_SYMLINKS { |
There was a problem hiding this comment.
The function resolve_symlink_path lacks documentation explaining its purpose, parameters, and return value. Given its complexity with symlink resolution and loop detection, it should include a docstring describing the MAX_SYMLINKS limit and loop detection mechanism.
|
|
||
| if st_size > old_size { | ||
| handle | ||
| .seek(std::io::SeekFrom::Start(old_size)) | ||
| .await | ||
| .map_err(map_io_err)?; | ||
|
|
||
| let mut remaining = st_size - old_size; | ||
| let zeros = [0u8; 8192]; | ||
| while remaining > 0 { | ||
| let chunk = std::cmp::min(remaining, zeros.len() as u64) as usize; | ||
| handle | ||
| .write_all(&zeros[..chunk]) | ||
| .await | ||
| .map_err(map_io_err)?; | ||
| remaining -= chunk as u64; | ||
| } | ||
|
|
||
| handle | ||
| .seek(std::io::SeekFrom::Start(original_offset)) | ||
| .await | ||
| .map_err(map_io_err)?; | ||
| } | ||
|
|
There was a problem hiding this comment.
The file extension logic manually writes zeros in 8KB chunks. Consider using set_len or a platform-specific sparse file API to extend the file more efficiently, especially for large files where writing zeros could be slow.
| if st_size > old_size { | |
| handle | |
| .seek(std::io::SeekFrom::Start(old_size)) | |
| .await | |
| .map_err(map_io_err)?; | |
| let mut remaining = st_size - old_size; | |
| let zeros = [0u8; 8192]; | |
| while remaining > 0 { | |
| let chunk = std::cmp::min(remaining, zeros.len() as u64) as usize; | |
| handle | |
| .write_all(&zeros[..chunk]) | |
| .await | |
| .map_err(map_io_err)?; | |
| remaining -= chunk as u64; | |
| } | |
| handle | |
| .seek(std::io::SeekFrom::Start(original_offset)) | |
| .await | |
| .map_err(map_io_err)?; | |
| } |
| if !is_stdio { | ||
| let guard = inode.read(); | ||
| match guard.deref() { | ||
| Kind::Dir { .. } | Kind::Root { .. } => return Ok(Err(Errno::Isdir)), | ||
| Kind::PipeTx { .. } => return Ok(Err(Errno::Badf)), | ||
| Kind::File { .. } => { | ||
| if fd_entry.open_flags & crate::fs::Fd::READ == 0 { | ||
| return Ok(Err(Errno::Badf)); | ||
| } |
There was a problem hiding this comment.
The rights checking logic is now split between the File case (lines 154-160) and the default case (lines 161-165). Consider extracting this into a helper function to reduce duplication and improve maintainability.
| fn find_function_in_table( | ||
| &self, | ||
| store: &mut impl AsStoreMut, | ||
| func: &Function, | ||
| ) -> Option<u32> { | ||
| let table = &self.indirect_function_table; | ||
| let target_raw = unsafe { Value::FuncRef(Some(func.clone())).as_raw(store).funcref }; | ||
| let size = table.size(store); | ||
| for index in 0..size { | ||
| if let Some(Value::FuncRef(Some(existing))) = table.get(store, index) { | ||
| let existing_raw = unsafe { Value::FuncRef(Some(existing)).as_raw(store).funcref }; | ||
| if existing_raw == target_raw { | ||
| return Some(index); | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
The function performs a linear search through the entire function table. For modules with many functions, this could become a bottleneck. Consider maintaining a HashMap of function pointers to indices for O(1) lookups.
| let Some(rx_end) = self.rx_end.upgrade() else { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::BrokenPipe, | ||
| "PipeRx is closed", | ||
| )); | ||
| }; | ||
| { | ||
| let rx = rx_end.lock().unwrap(); | ||
| if rx.pending_bytes.saturating_add(buf.len()) > PIPE_MAX_BYTES { | ||
| return Err(std::io::ErrorKind::WouldBlock.into()); | ||
| } | ||
| } | ||
| self.send_bytes(buf) |
There was a problem hiding this comment.
There's a race condition between checking pending_bytes (line 254) and calling send_bytes (line 258). Another thread could write to the pipe between these operations, causing the buffer to exceed PIPE_MAX_BYTES. The lock should be held across both operations.
| let Some(rx_end) = self.rx_end.upgrade() else { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::BrokenPipe, | |
| "PipeRx is closed", | |
| )); | |
| }; | |
| { | |
| let rx = rx_end.lock().unwrap(); | |
| if rx.pending_bytes.saturating_add(buf.len()) > PIPE_MAX_BYTES { | |
| return Err(std::io::ErrorKind::WouldBlock.into()); | |
| } | |
| } | |
| self.send_bytes(buf) | |
| let Some(ref tx) = self.tx else { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::BrokenPipe, | |
| "PipeTx is closed", | |
| )); | |
| }; | |
| let Some(rx_end) = self.rx_end.upgrade() else { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::BrokenPipe, | |
| "PipeRx is closed", | |
| )); | |
| }; | |
| { | |
| let mut rx = rx_end.lock().unwrap(); | |
| let new_pending = rx.pending_bytes.saturating_add(buf.len()); | |
| if new_pending > PIPE_MAX_BYTES { | |
| return Err(std::io::ErrorKind::WouldBlock.into()); | |
| } | |
| rx.pending_bytes = new_pending; | |
| } | |
| if let Err(_) = tx.send(buf.to_vec()) { | |
| if let Some(rx_end) = self.rx_end.upgrade() { | |
| let mut rx = rx_end.lock().unwrap(); | |
| rx.pending_bytes = rx.pending_bytes.saturating_sub(buf.len()); | |
| } | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::BrokenPipe, | |
| "PipeTx is closed", | |
| )); | |
| } | |
| self.mark_other_end_readable(); | |
| Ok(buf.len()) |
…d_advice, fd_close, proc_parent, proc_signals_sizes_get, thread_parallelism
…e, fd_allocate, fd_fdstat_set_rights, fd_filestat_set_size, fd_filestat_set_times, fd_seek, path_create_directory, path_filestat_get, path_filestat_set_times, path_open2, path_readlink, path_remove_directory, path_symlink, path_unlink_file
…2, proc_signals_get, sock_get_opt, sock_listen, sock_set_opt, sock_shutdown, sock_stats, thread_sleep
…imes, path_link, proc_raise, port_addr_clear, port_add_remove, port_route_clear, sock_bind
… port_addr_add, port_gateway_set, port_route_remove, port_unbridge, resolve, sock_accept
…, path_open2, sock_connect, sock_join_multicast, sock_pair, sock_recv_from, sock_send_to, thread_join, thread_spawn
…wn2, proc_exec, proc_exec2, proc_exec3, proc_exec3_errors, proc_exec3_empty_argv, proc_snapshot, sock_send_to, sock_send_file
artemyarulin
force-pushed
the
tests/wasix-tests
branch
from
4ee8df9 to
d239123
Compare
|
Python upstream result: NO_CHANGE Baseline: main @ 0e9da5e Summary:
Results commit:
More info:
|
|
Python upstream result: REGRESSION Baseline: main @ 19299bc Summary:
Results commit:
Top regressions:
Top improvements:
Top removed tests:
More info:
|
As we've discussed - this PR uses a lot of open source project like LTP (Linux Test Project) as a basis for new WASIX tests. The idea is cover all the syscalls and ensure POSIX compatibility and also achieve great test coverage on WASIX.
This is the draft PR as we've agreed I'll slowly convert all the tests and once all syscalls are covered then I'll open it for the review.And we are done 🎉 Next step is to split it into much smaller chunks (probably one PR per syscall) and start slowly reviewing that.
Total syscalls: 135
Tested syscalls: 133
Untested syscalls: 2
Coverage: 98.5%
Syscalls tested: 133/135 (98.5%)
Total tests added: 769 (149 Rust + 620 C)
Total lines added: 23,211