Skip to content

Propagate deserialization errors for IpcSharedMemory #392

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

Merged
merged 2 commits into from
May 1, 2025

Conversation

simonwuelker
Copy link
Contributor

@simonwuelker simonwuelker commented May 1, 2025

This makes IpcSharedMemory::deserialize return an error if the deserialized data references an shared memory region region that does not exist.

Part of servo/servo#36792. This doesn't fix the actual issue, but gives users of ipc-channel the opportunity to handle the error gracefully. Servo is technically at fault for panicking now, because serialized data being corrupted is not unrealistic.

Comment on lines +50 to +53
thread::Builder::new()
.name("router-proxy".to_string())
.spawn(move || Router::new(msg_receiver, wakeup_receiver).run())
.expect("Failed to spawn router proxy thread");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave the router proxy thread a name to make it easier to identify when it panics.

@simonwuelker simonwuelker changed the title Handle shmem errors Handle deserialization errors for IpcSharedMemory May 1, 2025
@simonwuelker simonwuelker changed the title Handle deserialization errors for IpcSharedMemory Propagate deserialization errors for IpcSharedMemory May 1, 2025
This makes debugging easier when panics occur inside the thread.

Signed-off-by: Simon Wülker <[email protected]>
Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the version to 0.20.0 as well? We haven't published a version with #374 yet.

@simonwuelker
Copy link
Contributor Author

simonwuelker commented May 1, 2025

Can you update the version to 0.20.0 as well? We haven't published a version with #374 yet.

What about #390? I suggest we merge this PR first and then #390.

@jdm jdm added this pull request to the merge queue May 1, 2025
Merged via the queue into servo:main with commit 4f8750c May 1, 2025
16 checks passed
@wusyong
Copy link
Member

wusyong commented May 3, 2025

Can you update the version to 0.20.0 as well? We haven't published a version with #374 yet.

What about #390? I suggest we merge this PR first and then #390.

Feel free to open another PR to bump the version as that one is outdated already.
But I suspected the change I made is still relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants