OpenSSL 3.2.0, QUIC, macOS, error 56 on connected UDP socket #23396

Sashan · 2024-01-26T13:01:28Z

current translate_msg() function attempts to set ->msg_name (and ->msg_namelen) with BIO's peer name (connection destination) regardless if underlying socket is connected or not. Such implementation uncovers differences in socket implementation between various OSes.

As we have learned hard way sendmsg() and sendmmsg() on OpenBSD and (MacOS too) fail to send messages with ->msg_name being set on connected socket. In such case the caller receives EISCON errro.

I think translate_msg() caller should provide a hint to indicate whether we deal with connected (or un-connected) socket. For connected sockets the peer's name should not be set/filled by translate_msg(). On the other hand if socket is un-connected, then translate_msg() must populate ->msg_name and ->msg_namelen members.

The caller can use getpeername(2) to see if socket is connected. If getpeername() succeeds then we must be dealing with connected socket and translate_msg() must not set ->msg_name and ->msg_namelen members. If getpeername(2) fails, then translate_msg() must provide peer's name (destination address) in ->msg_name and set ->msg_namelen accordingly.

The propposed fix introduces is_connected() function, which applies getpeername() to socket bound to BIO instance. The dgram_sendmmsg() uses is_connected() as a hint for translate_msg() function, so msghdr gets initialized with respect to socket state.

The change also modifies existing test/quic_client_test.c so it also covers the case of connected socket. To keep things simple we can introduce optional argument connect_first to ./quic_client_test function. Without connect_first the test run as usual. With connect_first the test creates and connects socket first. Then it passes such socket to BIO sub-system to perform QUIC connect test as usual.

Fixes #23251

Checklist

documentation is added or updated
tests are added or updated

hlandau · 2024-01-29T08:46:33Z

crypto/bio/bss_dgram.c

+    struct sockaddr_storage ss;
+    socklen_t ss_len = sizeof(ss);
+
+    return (getpeername(b->num, (struct sockaddr *)&ss, &ss_len) == 0);


I do not think we want to add a syscall to every BIO_sendmmsg invocation.

Whether a socket is connected is supposed to be tracked by BIO_CTRL_DGRAM_SET_CONNECTED (data->connected).

Sashan · 2024-01-29T09:48:00Z

On Mon, Jan 29, 2024 at 12:46:50AM -0800, Hugo Landau wrote: @hlandau requested changes on this pull request. __________________________________________________________________ In [1]crypto/bio/bss_dgram.c: > mh->msg_iov = iov; mh->msg_iovlen = 1; mh->msg_control = msg->local != NULL ? control : NULL; mh->msg_controllen = msg->local != NULL ? BIO_CMSG_ALLOC_LEN : 0; mh->msg_flags = 0; } + +static int is_connected(BIO *b) +{ + struct sockaddr_storage ss; + socklen_t ss_len = sizeof(ss); + + return (getpeername(b->num, (struct sockaddr *)&ss, &ss_len) == 0); I do not think we want to add a syscall to every BIO_sendmmsg invocation. Whether a socket is connected is supposed to be tracked by BIO_CTRL_DGRAM_SET_CONNECTED (data->connected).

yes, it makes sense. I'll send updated PR shortly. The new version calls `getpeername()` in `dgram_ctrl()` when `BIO_C_SET_FD` command. I think this is the kind of change Hugo expects.

crypto/bio/bss_dgram.c

test/quic_client_test.c

Sashan · 2024-01-29T16:39:58Z

On Mon, Jan 29, 2024 at 08:03:08AM -0800, Tom???? Mr??z wrote: @t8m requested changes on this pull request. __________________________________________________________________ In [1]crypto/bio/bss_dgram.c: > mh->msg_iov = iov; mh->msg_iovlen = 1; mh->msg_control = msg->local != NULL ? control : NULL; mh->msg_controllen = msg->local != NULL ? BIO_CMSG_ALLOC_LEN : 0; mh->msg_flags = 0; } + Please drop this spurious addition. __________________________________________________________________ In [2]test/quic_client_test.c: > + if (test_get_argument_count() > 0) { + mode = test_get_argument(0); + if (TEST_ptr(mode) && strcasecmp(mode, "connect_first") == 0) { + connect_socket(); + } + + if (cc_fd == INVALID_SOCKET) { + TEST_error("Could not create connected socket"); + return 0; + } + } Not sure if it is worth changing it (probably not) but it would be much easier to just make the test_quic_client to have 2 iterations. (ADD_ALL_TESTS(test_quic_client, 2);) In one iteration you would use a connected socket and in the other unconnected one.

I can give it a try, so we will be able to decide which variant will look better.

…

-- Reply to this email directly, [3]view it on GitHub, or [4]unsubscribe. You are receiving this because you were assigned. Message ID: ***@***.***> References 1. #23396 (comment) 2. #23396 (comment) 3. #23396 (review) 4. https://github.com/notifications/unsubscribe-auth/AACSZTZLZBREHJYOXVF6C3DYQ7B3ZAVCNFSM6AAAAABCMDUEU2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNBZGAZTGMRXGU

test/quic_client_test.c

t8m

Approving with or without the unresolved nit fixed.

Also please use git commit -a --fixup <commitid> instead of force pushes to make the incremental review easier.

crypto/bio/bss_dgram.c

hlandau · 2024-02-09T15:20:51Z

crypto/bio/bss_dgram.c

@@ -573,6 +575,10 @@ static long dgram_ctrl(BIO *b, int cmd, long num, void *ptr)
        b->shutdown = (int)num;
        b->init = 1;
        dgram_update_local_addr(b);
+	if (getpeername(b->num, (struct sockaddr *)&ss, &ss_len) == 0) {


indentation needs fixing in this file

Ah, @Sashan it seems your editor needs to be configured to replace tabs with spaces.

hlandau · 2024-02-09T15:21:07Z

crypto/bio/bss_dgram.c

+    data = (bio_dgram_data *)b->ptr;
+    if (data->connected == 0) {
+	/* macOS requires msg_namelen be 0 if msg_name is NULL */
+	mh->msg_name = msg->peer != NULL ? &msg->peer->sa : NULL;


indentation

hlandau · 2024-02-09T15:21:15Z

crypto/bio/bss_dgram.c

+	else
+	    mh->msg_namelen = 0;
+    } else {
+	mh->msg_name = NULL;


indentation

hlandau · 2024-02-09T15:21:29Z

test/quic_client_test.c

+					htons(port))))
+	    goto err;
+    } else {
+	c_fd = fd_arg;


indentation also in this file

openssl-machine · 2024-03-17T00:09:02Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

current `translate_msg()` function attempts to set `->msg_name` (and `->msg_namelen`) with `BIO`'s peer name (connection destination) regardless if underlying socket is connected or not. Such implementation uncovers differences in socket implementation between various OSes. As we have learned hard way `sendmsg()` and `sendmmsg()` on `OpenBSD` and (`MacOS` too) fail to send messages with `->msg_name` being set on connected socket. In such case the caller receives `EISCON` errro. I think `translate_msg()` caller should provide a hint to indicate whether we deal with connected (or un-connected) socket. For connected sockets the peer's name should not be set/filled by `translate_msg()`. On the other hand if socket is un-connected, then `translate_msg()` must populate `->msg_name` and `->msg_namelen` members. The caller can use `getpeername(2)` to see if socket is connected. If `getpeername()` succeeds then we must be dealing with connected socket and `translate_msg()` must not set `->msg_name` and `->msg_namelen` members. If `getpeername(2)` fails, then `translate_msg()` must provide peer's name (destination address) in `->msg_name` and set `->msg_namelen` accordingly. The propposed fix introduces `is_connected()` function, which applies `getpeername()` to socket bound to `BIO` instance. The `dgram_sendmmsg()` uses `is_connected()` as a hint for `translate_msg()` function, so msghdr gets initialized with respect to socket state. The change also modifies existing `test/quic_client_test.c` so it also covers the case of connected socket. To keep things simple we can introduce optional argument `connect_first` to `./quic_client_test` function. Without `connect_first` the test run as usual. With `connect_first` the test creates and connects socket first. Then it passes such socket to `BIO` sub-system to perform `QUIC` connect test as usual. Fixes openssl#23251

- fixes tabs vs. spaces pointed by hlandau

openssl-machine · 2024-04-16T14:00:15Z

This pull request is ready to merge

t8m · 2024-04-16T14:37:49Z

Merged to the master, 3.3 and 3.2 branches. Thank you.

current `translate_msg()` function attempts to set `->msg_name` (and `->msg_namelen`) with `BIO`'s peer name (connection destination) regardless if underlying socket is connected or not. Such implementation uncovers differences in socket implementation between various OSes. As we have learned hard way `sendmsg()` and `sendmmsg()` on `OpenBSD` and (`MacOS` too) fail to send messages with `->msg_name` being set on connected socket. In such case the caller receives `EISCON` errro. I think `translate_msg()` caller should provide a hint to indicate whether we deal with connected (or un-connected) socket. For connected sockets the peer's name should not be set/filled by `translate_msg()`. On the other hand if socket is un-connected, then `translate_msg()` must populate `->msg_name` and `->msg_namelen` members. The caller can use `getpeername(2)` to see if socket is connected. If `getpeername()` succeeds then we must be dealing with connected socket and `translate_msg()` must not set `->msg_name` and `->msg_namelen` members. If `getpeername(2)` fails, then `translate_msg()` must provide peer's name (destination address) in `->msg_name` and set `->msg_namelen` accordingly. The propposed fix introduces `is_connected()` function, which applies `getpeername()` to socket bound to `BIO` instance. The `dgram_sendmmsg()` uses `is_connected()` as a hint for `translate_msg()` function, so msghdr gets initialized with respect to socket state. The change also modifies existing `test/quic_client_test.c` so it also covers the case of connected socket. To keep things simple we can introduce optional argument `connect_first` to `./quic_client_test` function. Without `connect_first` the test run as usual. With `connect_first` the test creates and connects socket first. Then it passes such socket to `BIO` sub-system to perform `QUIC` connect test as usual. Fixes #23251 Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23396) (cherry picked from commit c062403)

current `translate_msg()` function attempts to set `->msg_name` (and `->msg_namelen`) with `BIO`'s peer name (connection destination) regardless if underlying socket is connected or not. Such implementation uncovers differences in socket implementation between various OSes. As we have learned hard way `sendmsg()` and `sendmmsg()` on `OpenBSD` and (`MacOS` too) fail to send messages with `->msg_name` being set on connected socket. In such case the caller receives `EISCON` errro. I think `translate_msg()` caller should provide a hint to indicate whether we deal with connected (or un-connected) socket. For connected sockets the peer's name should not be set/filled by `translate_msg()`. On the other hand if socket is un-connected, then `translate_msg()` must populate `->msg_name` and `->msg_namelen` members. The caller can use `getpeername(2)` to see if socket is connected. If `getpeername()` succeeds then we must be dealing with connected socket and `translate_msg()` must not set `->msg_name` and `->msg_namelen` members. If `getpeername(2)` fails, then `translate_msg()` must provide peer's name (destination address) in `->msg_name` and set `->msg_namelen` accordingly. The propposed fix introduces `is_connected()` function, which applies `getpeername()` to socket bound to `BIO` instance. The `dgram_sendmmsg()` uses `is_connected()` as a hint for `translate_msg()` function, so msghdr gets initialized with respect to socket state. The change also modifies existing `test/quic_client_test.c` so it also covers the case of connected socket. To keep things simple we can introduce optional argument `connect_first` to `./quic_client_test` function. Without `connect_first` the test run as usual. With `connect_first` the test creates and connects socket first. Then it passes such socket to `BIO` sub-system to perform `QUIC` connect test as usual. Fixes #23251 Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23396)

Sashan requested review from t8m and hlandau January 26, 2024 13:01

Sashan added triaged: bug The issue/pr is/fixes a bug tests: present The PR has suitable tests present labels Jan 26, 2024

arapov assigned Sashan Jan 28, 2024

hlandau requested changes Jan 29, 2024

View reviewed changes

Sashan force-pushed the issue.23251 branch 2 times, most recently from f86585c to f274cef Compare January 29, 2024 12:21

Sashan requested a review from hlandau January 29, 2024 13:34

t8m requested changes Jan 29, 2024

View reviewed changes

crypto/bio/bss_dgram.c Outdated Show resolved Hide resolved

test/quic_client_test.c Outdated Show resolved Hide resolved

t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member branch: 3.2 Merge to openssl-3.2 labels Jan 29, 2024

Sashan force-pushed the issue.23251 branch 2 times, most recently from 6e44fcd to 664e8d9 Compare January 29, 2024 17:15

t8m reviewed Jan 29, 2024

View reviewed changes

test/quic_client_test.c Outdated Show resolved Hide resolved

t8m requested changes Jan 29, 2024

View reviewed changes

test/quic_client_test.c Outdated Show resolved Hide resolved

Sashan force-pushed the issue.23251 branch from 664e8d9 to ea48be0 Compare January 29, 2024 18:31

Sashan requested a review from t8m January 29, 2024 22:23

t8m approved these changes Jan 30, 2024

View reviewed changes

crypto/bio/bss_dgram.c Outdated Show resolved Hide resolved

t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jan 30, 2024

t8m approved these changes Jan 30, 2024

View reviewed changes

hlandau requested changes Feb 9, 2024

View reviewed changes

Sashan force-pushed the issue.23251 branch from e14e17f to 0a443d9 Compare February 12, 2024 11:05

t8m approved these changes Feb 12, 2024

View reviewed changes

Sashan requested a review from hlandau February 15, 2024 11:50

Sashan and others added 3 commits April 15, 2024 01:08

fixup! OpenSSL 3.2.0, QUIC, macOS, error 56 on connected UDP socket

1c2cccf

fixup! OpenSSL 3.2.0, QUIC, macOS, error 56 on connected UDP socket

f424064

- fixes tabs vs. spaces pointed by hlandau

Sashan force-pushed the issue.23251 branch from 0a443d9 to f424064 Compare April 14, 2024 23:09

nhorman approved these changes Apr 15, 2024

View reviewed changes

t8m added approval: done This pull request has the required number of approvals branch: 3.3 Merge to openssl-3.3 and removed approval: review pending This pull request needs review by a committer labels Apr 15, 2024

t8m approved these changes Apr 15, 2024

View reviewed changes

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 16, 2024

t8m closed this Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenSSL 3.2.0, QUIC, macOS, error 56 on connected UDP socket #23396

OpenSSL 3.2.0, QUIC, macOS, error 56 on connected UDP socket #23396

Sashan commented Jan 26, 2024 •

edited

hlandau Jan 29, 2024

Sashan commented Jan 29, 2024 via email

Sashan commented Jan 29, 2024 via email

t8m left a comment

hlandau Feb 9, 2024

t8m Feb 9, 2024

hlandau Feb 9, 2024

hlandau Feb 9, 2024

hlandau Feb 9, 2024

openssl-machine commented Mar 17, 2024

openssl-machine commented Apr 16, 2024

t8m commented Apr 16, 2024

OpenSSL 3.2.0, QUIC, macOS, error 56 on connected UDP socket #23396

OpenSSL 3.2.0, QUIC, macOS, error 56 on connected UDP socket #23396

Conversation

Sashan commented Jan 26, 2024 • edited

Checklist

hlandau Jan 29, 2024

Choose a reason for hiding this comment

Sashan commented Jan 29, 2024 via email

Sashan commented Jan 29, 2024 via email

t8m left a comment

Choose a reason for hiding this comment

hlandau Feb 9, 2024

Choose a reason for hiding this comment

t8m Feb 9, 2024

Choose a reason for hiding this comment

hlandau Feb 9, 2024

Choose a reason for hiding this comment

hlandau Feb 9, 2024

Choose a reason for hiding this comment

hlandau Feb 9, 2024

Choose a reason for hiding this comment

openssl-machine commented Mar 17, 2024

openssl-machine commented Apr 16, 2024

t8m commented Apr 16, 2024

Sashan commented Jan 26, 2024 •

edited