Add socketpair shim #3609

tiif · 2024-05-16T23:52:23Z

Fixes #3442
Design proposal: https://hackmd.io/@tiif/Skhc1t0-C

src/shims/unix/socket.rs

tests/pass-dep/libc/libc-socketpair.rs

tiif · 2024-05-19T14:30:29Z

This is not entirely ready, and still waiting for discussion to happen, but I will tag it with waiting-on-review for more visibility.

@rustbot ready

oli-obk

This will also need to handle the case where the other socket was dropped. What do the manpages say about that case? Will we need to report https://doc.rust-lang.org/std/io/enum.ErrorKind.html#variant.ConnectionAborted or something?

src/shims/unix/socket.rs

RalfJung

Thanks! I think the general strategy works, I left some comments to complement Oli's feedback.

There is one issue we need to still consider: the interaction with data race detection. Since this can be used to send data from one thread to another, arguably a write to a buffer should be considered a "release" operation, and a read should be considered an "acquire". The way to test for this is to have a test with -Zmiri-preemption-rate=0 and then

have a global static mut VAL: u8 = 0;
in main, create a socketpair and then spawn a thread
in the main thread, go on by setting VAL to 1 and then sending a byte onto one socket. then call thread::yield_now.
in the other thread, read one byte from the socket, then assert_eq!(VAL, 1).

In your PR as-is, this should report UB due to a data race. However, arguably there is no data race here, since the other thread waited to receive a signal from the main thread.

To fix that, you should move the VecDeque<u8> into a new type, Buffer. This buffer needs a second field clock: VClock.

When doing a write, you need to do something like clock.join(data_race.release_clock(ecx.get_active_thread(), ecx.machine.current_span())), where data_race is in ecx.machine.data_race but it can be None (in which case you don't need to do anything).
When doing a read, you need to data_race.acquire_clock(&buffer.clock, ecx.get_active_thread()).

(We should really have some nicer wrapper functions for this...)

src/shims/unix/socket.rs

tests/pass-dep/libc/libc-socketpair.rs

tiif · 2024-05-23T18:30:27Z

Hmm, it seems like I got into some rebase trouble. Sorry if anyone get pinged on this PR.

RalfJung · 2024-05-24T06:00:26Z

@rustbot author
(at least I think that reflects the current state)

src/shims/unix/socket.rs

tiif · 2024-05-25T00:07:43Z

It turns out that x86_64-apple-darwin does not support SOCK_NONBLOCK and SOCK_CLOEXEC. Details are posted here #3442 (comment)

But we have quite a few tasks that should be handled first, maybe we can delay the support x86_64-apple-darwin to another PR?

tiif · 2024-05-25T07:04:07Z

Don't we have to indicate EOF at some point in read? Like, when the other side got closed and all data has been read, or so?

This will also need to handle the case where the other socket was dropped. What do the manpages say about that case? Will we need to report https://doc.rust-lang.org/std/io/enum.ErrorKind.html#variant.ConnectionAborted or something?

Yes we need special handling for these cases¹, this is the new specification:

For non-blocking

read
- if write end open but no data available
  - fail with EAGAIN/EWOULDBLOCK
- if write end close
  - if buffer is not empty
    - read as much as we need/can and return number of bytes read
  - else (buffer is empty )
    - return 0
- if read size < available data
  - read as requested and return
- if read size >= available data
  - read as much as we can and return (partial read)
write
- if read end open
  - if write size <= buffer size
    - if there is available space to write all requested bytes
      - write and return
    - else (no available space to write all requested bytes)
      - fail with EAGAIN/EWOULDBLOCK
  - if write size > buffer size
    - if available space in buffer is 0
      - fail with EAGAIN/EWOULDBLOCK
    - else (available space in buffer != 0)
      - write as much as we can (partial write)
- if read end closed
  - fail with EPIPE

For blocking

read
- if write end open but no data available
  - block
- if write end close
  - if buffer is not empty
    - read as much as we need/can and return number of bytes read
  - else (buffer is empty )
    - return 0
- if read size < available data
  - read as requested and return
- if read size >= available data
  - read as much as we can and return
write
- if read end open
  - if write size <= buffer size
    - block until there is sufficient space to write all requested bytes
  - if write size > buffer size
    - block until there is sufficient space to write all requested bytes
- if read end close
  - fail with EPIPE

https://man7.org/linux/man-pages/man7/pipe.7.html PIPE_BUF section, I can't find exact specification for sockets, I assumed behaviour of NONBLOCKING file descriptors (sockets/pipe/FIFO) are the same here ↩

tiif · 2024-05-25T08:10:16Z

We also need to implement close.

We can add in a new field that is being shared between 2 socketpair struct:

struct SocketPair {
    readbuf: Rc<RefCell<VecDeque<u8>>>,
    writebuf: Rc<RefCell<VecDeque<u8>>>,
    is_nonblock: bool,
    both_end_open: Rc<RefCell<bool>>,
}

This is the initial state where both side is opened,

write to fd[0]  -----> write to buffer0 ----> read from fd[1] 
read from fd[0] <----- write to buffer1 <---- write to fd[1]

When fd[1] is closed, this will be the state of the socketpair:

write to fd[0]  -----> write to buffer0 ----> X
read from fd[0] <----- write to buffer1 <---- X

fd[1] should set both_end_open to false to notify fd[0] that the other end has been dropped.

oli-obk · 2024-05-25T10:50:11Z

How is nonblocking EOF differentiated from no available bytes?

RefCell<bool>

Prefer Cell when the wrapped type is simple and Copy like bool

oli-obk · 2024-05-25T10:59:24Z

Blocking write/read will require some refactorings to allow us to park the thread and continue later. We already have the concept for sleep, something similar will be needed here. I would recommend doing so in a separate PR and just keep emitting unsupported where it would block

oli-obk · 2024-05-25T12:44:24Z

We can add in a new field that is being shared between 2 socketpair struct:

That's not going to work with duplicated file descriptors, we'd need another way to detect that the last copy of one end of the pair was dropped.

What we could do instead is to use Weak instead of Rc for the writer. Once the corresponding reader is dropped, the weak cannot be upgraded to an rc anymore, signalling that the other end is gone.

This does mean you need to upgrade before every write, but that seems ok

tiif · 2024-05-25T13:30:57Z

What we could do instead is to use Weak instead of Rc for the writer. Once the corresponding reader is dropped, the weak cannot be upgraded to an rc anymore, signalling that the other end is gone.

Ah, that's neat! Since not being able to upgrade to Rc means the other side is dropped, we only need

struct SocketPair {
    readbuf: Weak<RefCell<VecDeque<u8>>>,
    writebuf: Weak<RefCell<VecDeque<u8>>>,
    is_nonblock: bool,
}

(Note: this won't be the final design, we still need to consider data race detection)

oli-obk · 2024-05-25T13:47:07Z

readbuf needs to be Rc, not Weak, one side always needs to be Rc, otherwise both Weaks are dangling and can't be upgraded. When thr last Rc gets dropped, all weak become invalid

tiif · 2024-05-25T14:43:19Z

readbuf needs to be Rc, not Weak, one side always needs to be Rc, otherwise both Weaks are dangling and can't be upgraded.

I see.

Let's say fd[1] is dropped,

(type: Weak) write to fd[0]  -----> write to buffer0 ----> X
(type: Rc) read from fd[0] <----- write to buffer1 <----  X <-- consider this direction

we could use Rc::weak_count to track if the write end is dropped?

oli-obk · 2024-05-25T15:01:35Z

we could use Rc::weak_count to track if the write end is dropped?

Yea, that would work

RalfJung · 2024-05-26T07:20:52Z

@tiif your list above is not entirely right -- when all writers are closed but there is data left in the buffer, that data should still be returned. Only then should you indicate EOF.

RalfJung · 2024-05-26T07:22:34Z

Checking the counts is suspicious, those are pretty hard to use as a signal for anything... if we ever add some other reference to this buffer then suddenly this logic will break.

Maybe it's better to just have our own has_writer flag that we track explicitly? close on a Socketpair can then set has_writer on its write buffer to false. (close will only be called when the last FD for this file description is closed.)

tiif · 2024-05-26T17:01:27Z

when all writers are closed but there is data left in the buffer, that data should still be returned.

Yes, I didn't consider this case. The list is now updated.

Maybe it's better to just have our own has_writer flag that we track explicitly? close on a Socketpair can then set has_writer on its write buffer to false. (close will only be called when the last FD for this file description is closed.)

I have been thinking about this for a whlie, but not sure how this is going to work yet.

Initial state, fd[0] is the writer of its writebuf, and fd[1] is the writer of fd[0]'s readbuf.

write to fd[0]  -----> write to buffer0 ----> read from fd[1] 
read from fd[0] <----- write to buffer1 <---- write to fd[1]

If we dup fd[0] , and only consider buffer0

write to fd[0]  ----->    |----------------------|
                          | buffer0              |----> read from fd[1] 
write to newfd  ----->    |----------------------|

Closing fd[0] is a valid action and it will just make the state transit to:

write to newfd  -----> write to buffer0 ----> read from fd[1]

So I am not sure how close can only be called when the last FD for this file description is closed.

I am thinking of letting the socketpair having refcount for writers of its writebuf and readbuf.

struct SocketPair {
    readbuf: Rc<RefCell<VecDeque<u8>>>,
    writebuf: Weak<RefCell<VecDeque<u8>>>,
    is_nonblock: bool,
    // number of writer for readbuf
    readbufWriterCount: Rc<Cell<i64>>,
    // number of writer for writebuf
    writebufWriterCount: Rc<Cell<i64>>
}

write to fd[0]  ----->    |----------------------|
                          | buffer0              |----> read from fd[1] 
write to newfd  ----->    |----------------------|

For this case, writebufWriterCount of fd[0] and newfd hold the same reference as readbufWriterCount of fd[1]
When fd[0] is dup to newfd, writebufWriterCount of fd[0] and newfd will increase by 1 (So readbufWriterCount of fd[1] will increase by 1 too). To check if all the write end of the readbuf is dropped, we only need to check if the value of readbufWriterCount is 0.

Since these counters is only used to track writer of buffers, it won't break when the buffer is referred for other purpose.

Not sure if I over-complicated this problem, I feel a more elegant solution exists somewhere.

RalfJung

Thanks a lot! I have a bunch of comments but they're mostly minor. :)

src/shims/unix/socket.rs

tests/fail-dep/libc/socketpair_read_blocking.rs

RalfJung · 2024-06-09T07:57:50Z

@rustbot author

tiif · 2024-06-09T11:12:20Z

@rustbot ready

RalfJung

We're getting there. :)

src/shims/unix/socket.rs

RalfJung · 2024-06-10T11:24:08Z

@rustbot author

tiif · 2024-06-10T12:36:07Z

Thanks for the thorough review ;)

@rustbot ready

RalfJung · 2024-06-10T13:16:29Z

src/shims/unix/socket.rs

-        let mut mut_readbuf = self.readbuf.borrow_mut();
+        let mut readbuf = self.readbuf.borrow_mut();
+        let data_size: usize = readbuf.buf.len();
+        let readbuf_has_writer = readbuf.buf_has_writer;


Both of these let-bindings are used exactly once. Are they worth having as separate variables here or does the code become cleaner when they get inlined?

In fact data_size is just used in a == 0 check so that could become if readbuf.buf.is_empty().

RalfJung · 2024-06-10T13:17:21Z

src/shims/unix/socket.rs

-        let data_size = writebuf.borrow().buf.len();
-        let mut mut_writebuf = writebuf.borrow_mut();
+        let mut writebuf = writebuf.borrow_mut();
+        let data_size = writebuf.buf.len();
        let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.checked_sub(data_size).unwrap();
        let actual_write_size = write_size.min(available_space);


This let can be moved down, it's not used until the extend below.

RalfJung · 2024-06-10T13:18:01Z

Just two more nits. :)

Please squash after resolving them.

tiif · 2024-06-10T15:54:10Z

Thanks, just a heads-up, I did rebase + squash again because the git log is quite a mess, I will find a better workflow soon.

RalfJung · 2024-06-10T15:55:08Z

@bors r+

Thanks, just a heads-up, I did rebase + squash again because the git log is quite a mess, I will find a better workflow soon.

What I usually do is look at git log to find the most recent bors commit, copy-paste that commit ID, and then do git rebase --interactive <that commit id> to squash.

bors · 2024-06-10T15:55:11Z

📌 Commit 8732443 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-06-10T15:56:23Z

⌛ Testing commit 8732443 with merge 65f3e90...

tiif · 2024-06-10T16:00:38Z

Thanks, I will try that next time.

bors · 2024-06-10T16:25:43Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 65f3e90 to master...

tiif commented May 17, 2024

View reviewed changes

src/shims/unix/socket.rs Outdated Show resolved Hide resolved

src/shims/unix/socket.rs Outdated Show resolved Hide resolved

tiif commented May 17, 2024

View reviewed changes

tests/pass-dep/libc/libc-socketpair.rs Outdated Show resolved Hide resolved

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label May 19, 2024

oli-obk requested changes May 22, 2024

View reviewed changes

RalfJung reviewed May 22, 2024

View reviewed changes

tiif force-pushed the feat/socketpair branch from c676b6e to ce9fb54 Compare May 23, 2024 19:40

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels May 24, 2024

RalfJung reviewed May 24, 2024

View reviewed changes

src/shims/unix/socket.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 24, 2024

View reviewed changes

src/shims/unix/socket.rs Show resolved Hide resolved

RalfJung reviewed May 24, 2024

View reviewed changes

src/shims/unix/socket.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

tiif force-pushed the feat/socketpair branch from 3741f31 to d6d527c Compare May 26, 2024 08:17

RalfJung reviewed Jun 8, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Jun 9, 2024

tiif force-pushed the feat/socketpair branch from e3ce237 to 8dcf435 Compare June 9, 2024 11:08

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jun 9, 2024

RalfJung reviewed Jun 10, 2024

View reviewed changes

src/shims/unix/socket.rs Outdated Show resolved Hide resolved

src/shims/unix/socket.rs Outdated Show resolved Hide resolved

src/shims/unix/socket.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jun 10, 2024

View reviewed changes

src/shims/unix/socket.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Jun 10, 2024

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jun 10, 2024

RalfJung reviewed Jun 10, 2024

View reviewed changes

Add socketpair shim

8732443

tiif force-pushed the feat/socketpair branch from 03ea049 to 8732443 Compare June 10, 2024 15:51

bors merged commit 65f3e90 into rust-lang:master Jun 10, 2024
8 checks passed

tiif deleted the feat/socketpair branch June 10, 2024 16:47

tiif mentioned this pull request Jul 13, 2024

Implement pipe/pipe2 #3746

Closed

Add socketpair shim #3609

Add socketpair shim #3609

Conversation

tiif commented May 16, 2024 • edited Loading

tiif commented May 19, 2024 • edited Loading

oli-obk left a comment

Choose a reason for hiding this comment

RalfJung left a comment

Choose a reason for hiding this comment

tiif commented May 23, 2024 • edited Loading

RalfJung commented May 24, 2024

tiif commented May 25, 2024 • edited Loading

tiif commented May 25, 2024 • edited Loading

For non-blocking

For blocking

Footnotes

tiif commented May 25, 2024

oli-obk commented May 25, 2024

oli-obk commented May 25, 2024

This comment was marked as resolved.

This comment was marked as resolved.

oli-obk commented May 25, 2024

tiif commented May 25, 2024 • edited Loading

oli-obk commented May 25, 2024

tiif commented May 25, 2024

oli-obk commented May 25, 2024

RalfJung commented May 26, 2024

RalfJung commented May 26, 2024 • edited Loading

tiif commented May 26, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Jun 9, 2024

tiif commented Jun 9, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Jun 10, 2024

tiif commented Jun 10, 2024

RalfJung Jun 10, 2024

Choose a reason for hiding this comment

RalfJung Jun 10, 2024

Choose a reason for hiding this comment

RalfJung commented Jun 10, 2024

tiif commented Jun 10, 2024

RalfJung commented Jun 10, 2024

bors commented Jun 10, 2024

bors commented Jun 10, 2024

tiif commented Jun 10, 2024

bors commented Jun 10, 2024

tiif commented May 16, 2024 •

edited

Loading

tiif commented May 19, 2024 •

edited

Loading

tiif commented May 23, 2024 •

edited

Loading

tiif commented May 25, 2024 •

edited

Loading

tiif commented May 25, 2024 •

edited

Loading

tiif commented May 25, 2024 •

edited

Loading

RalfJung commented May 26, 2024 •

edited

Loading