casaroli commented on code in PR #21403:
URL: https://github.com/apache/nuttx/pull/21403#discussion_r4453584650
##########
net/rpmsg/rpmsg_sockif.c:
##########
@@ -345,6 +351,20 @@ static int rpmsg_socket_ept_cb(FAR struct rpmsg_endpoint
*ept,
rpmsg_socket_post(&conn->sendsem);
rpmsg_socket_poll_notify(conn, POLLOUT);
nxmutex_unlock(&conn->sendlock);
+
+ /* Accept new connection and complete the final handshake */
+
+ if (conn->server)
Review Comment:
`conn->server` is a raw pointer to the listening socket, but
`rpmsg_socket_close()` frees a listening socket at once and does not know the
connections that wait for their SYNC. If the listener closes before the SYNC
arrives, this code locks `server->recvlock` and writes `tmp->next` in freed
memory.
Fix: keep the pending connections in a list that the server owns, and on
close of the listener unlink them and clear their `server` pointer under
`server->recvlock`.
##########
net/rpmsg/rpmsg_sockif.c:
##########
@@ -1517,20 +1546,47 @@ static int rpmsg_socket_setsockopt(FAR struct socket
*psock, int level,
{
FAR struct rpmsg_socket_conn_s *conn = psock->s_conn;
- if (level == SOL_SOCKET && option == SO_RCVBUF)
+ if (level == SOL_SOCKET)
{
- if (value_len < sizeof(int))
+ switch (option)
{
- return -EINVAL;
- }
+ case SO_RCVBUF:
+ {
+ if (value_len < sizeof(int))
+ {
+ return -EINVAL;
+ }
- if (_SS_ISCONNECTED(conn->sconn.s_flags))
- {
- return -EISCONN;
- }
+ if (_SS_ISCONNECTED(conn->sconn.s_flags))
+ {
+ return -EISCONN;
+ }
+
+ conn->recvsize = *(FAR const int *)value;
+ return OK;
+ }
+
+ case SO_PRIORITY:
+ {
+ if (value_len != sizeof(uint8_t))
Review Comment:
SO_PRIORITY takes an `int` value (socket(7)), but this code accepts only
`sizeof(uint8_t)`, so `setsockopt(fd, SOL_SOCKET, SO_PRIORITY, &prio,
sizeof(prio))` with `int prio` fails with EINVAL.
Fix: accept an `int`, check the range 0..RPMSG_PRIO_MAX-1, and store it in
`conn->priority`; the same in getsockopt (line 1529).
##########
net/rpmsg/rpmsg_sockif.c:
##########
@@ -1025,6 +1036,13 @@ static ssize_t rpmsg_socket_send_continuous(FAR struct
socket *psock,
if (block == 0)
{
+ if (written > 0)
Review Comment:
For a non-blocking socket the old code already returned `written` (`return
written ? written : ret`), so this only changes blocking sockets: a blocking
send() now returns a short count when the remote buffer fills. POSIX says a
blocking send() blocks until space is available, and callers that send once
lose the rest.
Fix: remove this `break`, or apply it only when `nonblock` is true.
##########
net/rpmsg/rpmsg_sockif.c:
##########
@@ -599,33 +627,28 @@ static void rpmsg_socket_ns_bind(FAR struct rpmsg_device
*rdev,
return;
}
+ nxmutex_lock(&server->recvlock);
+ if (server->pending >= server->backlog)
+ {
+ /* Reject the connection */
+
+ nerr("Reject the connection, Max listen %d\n", server->backlog);
+ nxmutex_unlock(&server->recvlock);
+ rpmsg_destroy_ept(&new->ept);
+ rpmsg_socket_free(new);
+ return;
+ }
+
+ server->pending++;
Review Comment:
`pending` goes up here but only accept() decrements it. When the client
unbinds before its SYNC, `rpmsg_socket_ns_unbind()` clears `conn->server`, the
conn is never queued, never accepted and never freed, and `pending` stays up.
After `backlog` such aborted handshakes, every new connection is rejected.
Fix: in `rpmsg_socket_ns_unbind()` (and on ept release) of a conn that is
not queued yet, decrement `server->pending` under `server->recvlock` and free
the conn.
--
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]
