search

rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.31k stars 327 forks source link

Implement minimal socket shim #3426

Closed ZoeS17 closed 5 months ago

ZoeS17 commented 5 months ago

I vaguely followed socketpair as to implementation details. I attempted to make a test too but there's not exactly a good way, that I can figure out, besides simply checking that an file descriptor gets made. I dd so for both the libc crate and the version I implemented.

Free feel to ask question and/or adjust if needed.

bors commented 5 months ago

:umbrella: The latest upstream changes (presumably #3441) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung commented 5 months ago

IOW, I think this should follow the proposed "project" process: https://github.com/rust-lang/miri/issues/3443. We're happy to help you through that process! This is probably best done on Zulip, but you can also open an issue dedicated to the project and we can discuss there.

RalfJung commented 5 months ago

I have opened an issue to track this: https://github.com/rust-lang/miri/issues/3449.

I think blocking sockets are actually highly non-trivial to implement.

RalfJung commented 5 months ago

So to be clear I think before any implementation work is done, we need a clear description of which part of the socket API is intended to be implemented (TCP/UDP/..., blocking/non-blocking, which shims are involved). And since this will be non-trivial to implement, we also need a description of what the implementation plan is -- which new state is needed in Miri, how do we handle waking up the right threads when the sockets they are blocked on get ready, things like that. I laid out some of the complications in https://github.com/rust-lang/miri/issues/3449.

Adding sockets is not an easy project. And with projects of this scale it's not a good idea to just go implement something -- that has a pretty high risk that we'll ask you to re-write it entirely as we see flaws with the fundamental structure of the approach, and it also makes review much harder when we have to reverse engineer the high-level approach from the code.

@ZoeS17 what is your goal with this PR? Is there a motivation for wanting to land just the socket shim without actually supporting sockets at all?

bors commented 5 months ago

:umbrella: The latest upstream changes (presumably #3451) made this pull request unmergeable. Please resolve the merge conflicts.

ZoeS17 commented 5 months ago

Adding sockets is not an easy project. And with projects of this scale it's not a good idea to just go implement something -- that has a pretty high risk that we'll ask you to re-write it entirely as we see flaws with the fundamental structure of the approach, and it also makes review much harder when we have to reverse engineer the high-level approach from the code.

Agreed.

what is your goal with this PR? Is there a motivation for wanting to land just the socket shim without actually supporting sockets at all?

My intent was originally to land this as a shim without supporting sockets at all but given what has been said in the #3449 I agree that it seems foolish to continue as a "hollow" implementation. I originally come upon the desire to pick up this work because I was attempting to run miri against a program I was building that utilizes sockets to connect to IRC. Since obviously that needs a deeper implementation then I originally understood I'm up for closing this PR with an understanding that I'm willing to admit I may be out of my depth here.

RalfJung commented 5 months ago

I originally come upon the desire to pick up this work because I was attempting to run miri against a program I was building that utilizes sockets to connect to IRC.

Ah, yes makes sense. I'm afraid that's a pretty hard problem and quite far off currently, unfortunately.

I'll close this PR then. Thanks anyway!