Conversation
There was a problem hiding this comment.
Pull request overview
This PR applies three bug fixes in the host runtime: (1) make net_getsockopt(SO_TYPE) reflect the actual socket type (STREAM vs DGRAM), (2) prevent concurrent VMs from racing while redirecting process-wide stderr, and (3) avoid silent socket-id wraparound by moving socket table IDs to u64.
Changes:
- Track and return the real socket type (including for accepted sockets) and update socket table IDs from
u32tou64. - Serialize stderr redirection/restoration with a process-wide mutex to avoid cross-VM
dup2races. - Make stderr capture temp file names unique per thread.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
host/src/stderr_capture.rs |
Adds a process-wide mutex and holds it across stderr redirection/restoration to prevent dup2 races between concurrent VMs. |
host/src/lib.rs |
Extends socket tracking to include socket type, returns correct SO_TYPE, updates socket IDs to u64, and makes capture temp filenames per-thread unique. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
27
to
33
| let guard = STDERR_LOCK | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
| let capture_fd = std::fs::File::create(path)?.into_raw_fd(); | ||
| let original_stderr_raw = unistd::dup(2)?; | ||
| unistd::dup2(capture_fd, 2)?; | ||
| unistd::close(capture_fd)?; |
Comment on lines
41
to
44
| pub fn restore(self) -> Result<()> { | ||
| unistd::dup2(self.original_stderr.as_raw_fd(), 2)?; | ||
| // _guard is dropped here, releasing STDERR_LOCK | ||
| Ok(()) |
Comment on lines
1340
to
1344
| let t = table.clone(); | ||
| tools.register("net_getsockopt", move |args| { | ||
| let fd = args["fd"].as_u64().ok_or_else(|| anyhow!("missing 'fd'"))? as u32; | ||
| let fd = args["fd"].as_u64().ok_or_else(|| anyhow!("missing 'fd'"))?; | ||
| let level = args["level"].as_i64().unwrap_or(0) as i32; | ||
| let optname = args["optname"].as_i64().unwrap_or(0) as i32; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Linux Benchmarks
Details
| Benchmark suite | Current: 3410596 | Previous: f5eb506 | Ratio |
|---|---|---|---|
hello_world (median) |
20 ms |
20 ms |
1 |
pandas (median) |
100 ms |
110 ms |
0.91 |
density (per VM) |
7 MB |
7 MB |
1 |
snapshot (disk) |
385 MiB |
385 MiB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
Signed-off-by: danbugs <danilochiarlone@gmail.com>
Signed-off-by: danbugs <danilochiarlone@gmail.com>
Signed-off-by: danbugs <danilochiarlone@gmail.com>
Signed-off-by: danbugs <danilochiarlone@gmail.com>
Signed-off-by: danbugs <danilochiarlone@gmail.com>
Signed-off-by: danbugs <danilochiarlone@gmail.com>
Contributor
There was a problem hiding this comment.
Windows Benchmarks
Details
| Benchmark suite | Current: 3410596 | Previous: f5eb506 | Ratio |
|---|---|---|---|
hello_world (median) |
257 ms |
221 ms |
1.16 |
pandas (median) |
808 ms |
697 ms |
1.16 |
density (per VM) |
6 MB |
6 MB |
1 |
snapshot (disk) |
393 MiB |
393 MiB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three independent bug fixes:
net_getsockopthardcoded SO_TYPE to 1 (SOCK_STREAM),but
net_socketaccepts type=2 (DGRAM). Now stores socket type inHostSocketat creation time and returns the actual value. Propagated throughnet_acceptas well.dup2(capture_fd, 2)is process-global — concurrentVMs race on fd 2. Added a process-wide
Mutexheld for the capture duration.Also made the temp file path unique per thread.
next_idfromu32tou64and allrelated fd parsing sites. Prevents silent wraparound after 2^32 inserts.
Test plan
HostSockettuple carries type through accept path