casaroli commented on code in PR #21418:
URL: https://github.com/apache/nuttx/pull/21418#discussion_r4459452388
##########
net/local/local_sockif.c:
##########
@@ -822,9 +832,36 @@ static int local_connect(FAR struct socket *psock,
#ifdef CONFIG_NET_LOCAL_DGRAM
case SOCK_DGRAM:
{
- /* Perform the datagram connection logic */
+ FAR struct local_conn_s *conn = psock->s_conn;
+ FAR const struct sockaddr_un *unaddr =
+ (FAR const struct sockaddr_un *)addr;
+
+ if (addr->sa_family == AF_UNSPEC)
+ {
+ memset(&conn->lc_daddr, 0, sizeof(conn->lc_daddr));
+ if (conn->lc_state == LOCAL_STATE_CONNECTED)
+ {
+ conn->lc_state = conn->lc_type == LOCAL_TYPE_UNNAMED
+ ? LOCAL_STATE_UNBOUND
+ : LOCAL_STATE_BOUND;
+ }
+
+ return OK;
+ }
+
+ /* Record the default peer. Copy only what the caller supplied
+ * so that a short addrlen does not read past its buffer.
+ */
- return -ENOSYS;
+ if (addrlen > sizeof(conn->lc_daddr))
+ {
+ addrlen = sizeof(conn->lc_daddr);
+ }
+
+ memset(&conn->lc_daddr, 0, sizeof(conn->lc_daddr));
+ memcpy(&conn->lc_daddr, unaddr, addrlen);
+ conn->lc_state = LOCAL_STATE_CONNECTED;
Review Comment:
connect() sets `lc_state` to `LOCAL_STATE_CONNECTED`, but `lc_peer` stays
NULL. `local_getpeername()` checks only `lc_state` and then reads
`peer->lc_type`, so getpeername() of a connected datagram socket is a NULL
dereference: data abort at FAR 0x52 on qemu-armv8a:nsh and :knsh.
Fix: in `local_getpeername()`, when `lc_peer` is NULL, give the address in
`lc_daddr`.
##########
net/local/local_netpoll.c:
##########
@@ -161,7 +161,7 @@ int local_pollsetup(FAR struct socket *psock, FAR struct
pollfd_s *fds)
if (conn->lc_proto == SOCK_DGRAM)
{
- return -ENOSYS;
+ return file_poll(&conn->lc_infile, fds, true);
Review Comment:
Each send opens and closes the receiver FIFO, and `pipecommon_close()` posts
POLLHUP at the close of the last writer also for buffer policy 1
(drivers/pipes/pipe_common.c:350). poll() of the receiver gives POLLIN|POLLHUP
for each datagram, and POLLHUP alone for a send that fails with EMSGSIZE. A
program that closes the socket on POLLHUP stops after the first datagram.
Fix: in `pipecommon_close()`, post POLLHUP only for policy 0, as
`pipecommon_poll()` does.
##########
net/local/local_bind.c:
##########
@@ -100,6 +100,27 @@ int psock_local_bind(FAR struct socket *psock,
strlcpy(conn->lc_path, &unaddr->sun_path[index], sizeof(conn->lc_path));
+#ifdef CONFIG_NET_LOCAL_DGRAM
+ if (psock->s_type == SOCK_DGRAM)
+ {
+ int ret;
+
+ ret = local_create_halfduplex(conn, conn->lc_path, conn->lc_rcvsize);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ ret = local_open_receiver(conn,
Review Comment:
bind() now opens the FIFO in the blocking mode of the socket, and
`psock_dgram_recvfrom()` uses MSG_DONTWAIT only when it opens the FIFO itself.
So recv(MSG_DONTWAIT) of an empty bound socket now waits forever (master:
EAGAIN).
Fix: in `psock_dgram_recvfrom()`, when `lc_infile` is open and MSG_DONTWAIT
is set, return -EAGAIN if FIONREAD gives 0, as `psock_stream_recvfrom()` does.
##########
net/local/local_sendmsg.c:
##########
@@ -286,11 +201,13 @@ static ssize_t local_sendto(FAR struct socket *psock,
/* Verify that this is not a connected peer socket. It need not be
* bound, however. If unbound, recvfrom will see this as a nameless
- * connection.
+ * connection. DGRAM connected state is allowed since connect() only
+ * sets the default peer address without establishing a FIFO link.
*/
if (conn->lc_state != LOCAL_STATE_UNBOUND &&
- conn->lc_state != LOCAL_STATE_BOUND)
+ conn->lc_state != LOCAL_STATE_BOUND &&
+ conn->lc_state != LOCAL_STATE_CONNECTED)
Review Comment:
POSIX connect(): for SOCK_DGRAM the peer address "limits the remote sender
for subsequent recv() functions". A connected receiver still takes datagrams
from every socket.
Fix: in `local_sendto()`, return -EPERM when the receiver has a peer address
that is not the name of the sender (Linux does this).
##########
net/local/local_conn.c:
##########
@@ -178,6 +178,17 @@ FAR struct local_conn_s *local_alloc(void)
nxmutex_init(&conn->lc_polllock);
nxrmutex_init(&conn->lc_conn.s_lock);
+ /* Give the connection a unique placeholder path. local_findconn()
+ * matches on lc_path, so leaving it empty would make every unbound
+ * connection look like every other one, and bind() would reject the
+ * second one with EADDRINUSE. lc_type stays LOCAL_TYPE_UNNAMED:
+ * the socket has no name until it is bound, and getsockname() and
+ * getpeername() rely on that to report a zero-length sun_path.
+ */
+
+ snprintf(conn->lc_path, sizeof(conn->lc_path),
+ "unnamed.%p", conn);
Review Comment:
The placeholder name goes out of the kernel. `local_send_preamble()` sends
`lc_path` as the source of each datagram, so recvfrom() of a datagram from an
unbound socket gives `unnamed.0x402ed1...` (master: no name). A stream client
gets `lc_type` LOCAL_TYPE_PATHNAME in `local_stream_connect()`, so
getsockname() of an unbound client and getpeername() of the accepted socket now
give `unnamed.0x402ec8f0` (master: empty). These are kernel heap addresses.
bind() does not need the placeholder: `psock_local_bind()` looks up the
requested name, and an empty `lc_path` matches only an empty name.
Fix: remove the placeholder.
##########
net/local/local_sockif.c:
##########
@@ -822,9 +832,36 @@ static int local_connect(FAR struct socket *psock,
#ifdef CONFIG_NET_LOCAL_DGRAM
case SOCK_DGRAM:
{
- /* Perform the datagram connection logic */
+ FAR struct local_conn_s *conn = psock->s_conn;
+ FAR const struct sockaddr_un *unaddr =
+ (FAR const struct sockaddr_un *)addr;
+
+ if (addr->sa_family == AF_UNSPEC)
+ {
+ memset(&conn->lc_daddr, 0, sizeof(conn->lc_daddr));
+ if (conn->lc_state == LOCAL_STATE_CONNECTED)
+ {
+ conn->lc_state = conn->lc_type == LOCAL_TYPE_UNNAMED
+ ? LOCAL_STATE_UNBOUND
+ : LOCAL_STATE_BOUND;
+ }
+
+ return OK;
+ }
+
+ /* Record the default peer. Copy only what the caller supplied
+ * so that a short addrlen does not read past its buffer.
+ */
- return -ENOSYS;
+ if (addrlen > sizeof(conn->lc_daddr))
+ {
+ addrlen = sizeof(conn->lc_daddr);
+ }
+
+ memset(&conn->lc_daddr, 0, sizeof(conn->lc_daddr));
+ memcpy(&conn->lc_daddr, unaddr, addrlen);
+ conn->lc_state = LOCAL_STATE_CONNECTED;
Review Comment:
connect() does not check the receiver: it returns 0 for a path that does not
exist (POSIX: ENOENT) and for a regular file (ECONNREFUSED). send() after the
receiver was closed returns ENOENT (Linux: ECONNREFUSED).
Fix: look up the receiver with `local_findconn()` in connect() and return
ENOENT or ECONNREFUSED as `psock_local_connect()` does; in `local_send()`,
return ECONNREFUSED for -ENOENT.
##########
net/local/local_bind.c:
##########
@@ -100,6 +100,27 @@ int psock_local_bind(FAR struct socket *psock,
strlcpy(conn->lc_path, &unaddr->sun_path[index], sizeof(conn->lc_path));
+#ifdef CONFIG_NET_LOCAL_DGRAM
+ if (psock->s_type == SOCK_DGRAM)
+ {
+ int ret;
+
+ ret = local_create_halfduplex(conn, conn->lc_path, conn->lc_rcvsize);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ ret = local_open_receiver(conn,
Review Comment:
A second bind() of a datagram socket opens `lc_infile` again over the open
file: the first FIFO keeps a reader that is never closed. If the FIFO cannot be
made, `lc_path` and `lc_type` stay set, and a retry gets EADDRINUSE from the
socket itself.
Fix: return -EINVAL when a datagram socket is already bound (Linux), and
clear `lc_path` and `lc_type` on the error path.
##########
net/local/local_sockif.c:
##########
@@ -822,9 +832,36 @@ static int local_connect(FAR struct socket *psock,
#ifdef CONFIG_NET_LOCAL_DGRAM
case SOCK_DGRAM:
{
- /* Perform the datagram connection logic */
+ FAR struct local_conn_s *conn = psock->s_conn;
+ FAR const struct sockaddr_un *unaddr =
+ (FAR const struct sockaddr_un *)addr;
+
+ if (addr->sa_family == AF_UNSPEC)
+ {
+ memset(&conn->lc_daddr, 0, sizeof(conn->lc_daddr));
+ if (conn->lc_state == LOCAL_STATE_CONNECTED)
+ {
+ conn->lc_state = conn->lc_type == LOCAL_TYPE_UNNAMED
+ ? LOCAL_STATE_UNBOUND
+ : LOCAL_STATE_BOUND;
+ }
+
+ return OK;
+ }
+
+ /* Record the default peer. Copy only what the caller supplied
+ * so that a short addrlen does not read past its buffer.
+ */
- return -ENOSYS;
+ if (addrlen > sizeof(conn->lc_daddr))
+ {
+ addrlen = sizeof(conn->lc_daddr);
+ }
+
+ memset(&conn->lc_daddr, 0, sizeof(conn->lc_daddr));
+ memcpy(&conn->lc_daddr, unaddr, addrlen);
Review Comment:
With an abstract name, send() after connect() returns success, but the
receiver gets nothing. `local_findconn()` skips the NUL at the start of
`sun_path`, but `local_sendto()` builds the FIFO name from `unaddr->sun_path`
(line 249), which is empty, so the datagram goes to the FIFO of the empty name.
Fix: in `local_sendto()`, use `&unaddr->sun_path[1]` for an abstract name,
as `local_findconn()` does.
##########
net/local/local_bind.c:
##########
@@ -100,6 +100,27 @@ int psock_local_bind(FAR struct socket *psock,
strlcpy(conn->lc_path, &unaddr->sun_path[index], sizeof(conn->lc_path));
+#ifdef CONFIG_NET_LOCAL_DGRAM
+ if (psock->s_type == SOCK_DGRAM)
+ {
+ int ret;
+
+ ret = local_create_halfduplex(conn, conn->lc_path, conn->lc_rcvsize);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ ret = local_open_receiver(conn,
+ _SS_ISNONBLOCK(conn->lc_conn.s_flags));
+ if (ret < 0)
+ {
+ local_release_halfduplex(conn);
+ return ret;
+ }
+ }
+#endif
+
conn->lc_state = LOCAL_STATE_BOUND;
Review Comment:
connect() and then bind() loses the peer: bind() sets `LOCAL_STATE_BOUND`,
and send() then fails with EDESTADDRREQ.
Fix: keep `LOCAL_STATE_CONNECTED` in bind() when the socket has a peer
address.
##########
net/local/local_sendmsg.c:
##########
@@ -393,6 +310,81 @@ static ssize_t local_sendto(FAR struct socket *psock,
#endif /* CONFIG_NET_LOCAL_DGRAM */
}
+/****************************************************************************
+ * Name: local_send
+ *
+ * Description:
+ * Send a local packet as a stream.
+ *
+ * Input Parameters:
+ * psock An instance of the internal socket structure.
+ * buf Data to send
+ * len Length of data to send
+ * flags Send flags (ignored for now)
+ *
+ * Returned Value:
+ * On success, returns the number of characters sent. On error,
+ * -1 is returned, and errno is set appropriately (see send() for the
+ * list of errno numbers).
+ *
+ ****************************************************************************/
+
+static ssize_t local_send(FAR struct socket *psock,
+ FAR const struct iovec *buf,
+ size_t len, int flags)
+{
+ ssize_t ret;
+
+ switch (psock->s_type)
+ {
+ case SOCK_STREAM:
+ {
+ FAR struct local_conn_s *conn = psock->s_conn;
+
+ DEBUGASSERT(buf);
+
+ if (conn->lc_state != LOCAL_STATE_CONNECTED)
+ {
+ nerr("ERROR: not connected\n");
+ return -ENOTCONN;
+ }
+
+ if (conn->lc_outfile.f_inode == NULL)
+ {
+ return -EPIPE;
+ }
+
+ ret = local_send_packet(&conn->lc_outfile, buf, len);
Review Comment:
The stream path no longer takes `lc_sendlock`, so two threads that send() on
one stream socket are not serialized any more. The PR body says that stream
sockets do not change.
Fix: keep `nxmutex_lock(&conn->lc_sendlock)` around `local_send_packet()`.
##########
net/local/local_netpoll.c:
##########
@@ -161,7 +161,7 @@ int local_pollsetup(FAR struct socket *psock, FAR struct
pollfd_s *fds)
if (conn->lc_proto == SOCK_DGRAM)
{
- return -ENOSYS;
+ return file_poll(&conn->lc_infile, fds, true);
Review Comment:
`lc_infile` is read-only, so poll() never reports POLLOUT for a datagram
socket; poll(POLLOUT) of a bound socket waits forever. An unbound socket has no
`lc_infile`: file_poll() returns -ENOSYS and poll() gives POLLERR.
Fix: for SOCK_DGRAM, always report POLLOUT (a send has no send buffer to
wait for), and call file_poll() for POLLIN only when `lc_infile` is open.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
