casaroli commented on code in PR #21403:
URL: https://github.com/apache/nuttx/pull/21403#discussion_r4459461187
##########
net/rpmsg/rpmsg_sockif.c:
##########
@@ -599,33 +734,35 @@ static void rpmsg_socket_ns_bind(FAR struct rpmsg_device
*rdev,
return;
}
- new->rpaddr.rp_family = AF_RPMSG;
- strlcpy(new->rpaddr.rp_cpu, rpmsg_get_cpuname(rdev),
- sizeof(new->rpaddr.rp_cpu));
- strlcpy(new->rpaddr.rp_name, name + RPMSG_SOCKET_NAME_PREFIX_LEN,
- sizeof(new->rpaddr.rp_name));
-
- rpmsg_socket_ns_bound(&new->ept);
-
nxmutex_lock(&server->recvlock);
- for (tmp = server; tmp->next; tmp = tmp->next)
+ if (server->pending >= server->backlog)
{
- if (++cnt >= server->backlog)
- {
- /* Reject the connection */
+ /* Reject the connection */
- nxmutex_unlock(&server->recvlock);
- rpmsg_destroy_ept(&new->ept);
- rpmsg_socket_free(new);
- return;
- }
+ nerr("Reject the connection, Max listen %d\n", server->backlog);
+ nxmutex_unlock(&server->recvlock);
+ rpmsg_destroy_ept(&new->ept);
+ rpmsg_socket_free(new);
+ return;
}
- tmp->next = new;
+ /* Count it against the backlog and let the listening socket own it until
+ * its SYNC arrives, so that the listener can detach it if it is closed
+ * in the meantime. The slot is released by accept(), by
+ * rpmsg_socket_ns_unbind() or when the listener is closed.
+ */
- rpmsg_socket_post(&server->recvsem);
- rpmsg_socket_poll_notify(server, POLLIN);
+ server->pending++;
+ rpmsg_socket_pend_add(server, new);
Review Comment:
The conn goes on the pend list only here, but `rpmsg_create_ept()` at line
728 already sent the NS ack, and the client answers with its SYNC. When ns_bind
runs from `listen()` (`rpmsg_register_callback()` binds the requests that came
before listen()), the rx thread can handle that SYNC first:
`rpmsg_socket_pend_remove()` fails, the SYNC is dropped, and the conn then
stays on the pend list: accept() never returns it and it holds a backlog slot.
Fix: do the backlog check, `pending++` and `rpmsg_socket_pend_add()` before
`rpmsg_create_ept()`, and undo them when `rpmsg_create_ept()` fails or the
connection is rejected.
##########
net/rpmsg/rpmsg_sockif.c:
##########
@@ -588,8 +714,17 @@ static void rpmsg_socket_ns_bind(FAR struct rpmsg_device
*rdev,
return;
}
+ new->server = server;
new->ept.priv = new;
new->ept.release_cb = rpmsg_socket_ept_release;
+
+ ret = rpmsg_set_priority(&new->ept, server->priority);
Review Comment:
The endpoint gets `server->priority`, but `new->priority` stays
`RPMSG_PRIO_DEFAULT`, so getsockopt(SO_PRIORITY) on the accepted socket returns
127 and not the priority in use.
Fix: set `new->priority = server->priority` and pass `new->priority` here.
##########
net/rpmsg/rpmsg_sockif.c:
##########
@@ -1363,11 +1488,35 @@ static int rpmsg_socket_close(FAR struct socket *psock)
if (conn->backlog > 0)
{
+ FAR struct rpmsg_socket_conn_s *pend;
+
rpmsg_unregister_callback(conn,
NULL,
NULL,
rpmsg_socket_ns_match,
rpmsg_socket_ns_bind);
+
+ /* Detach any connection still waiting for its SYNC: they hold a
+ * pointer to this listening socket which is about to be freed.
+ */
+
+ nxmutex_lock(&conn->recvlock);
+ while ((pend = conn->pendlist) != NULL)
+ {
+ conn->pendlist = pend->pendnext;
+ pend->pendnext = NULL;
+ conn->pending--;
+
+ nxmutex_lock(&pend->recvlock);
+ pend->server = NULL;
+ nxmutex_unlock(&pend->recvlock);
+
+ nxmutex_unlock(&conn->recvlock);
+ rpmsg_socket_destroy_ept(pend);
Review Comment:
A pending conn still has `crefs == 1` from `rpmsg_socket_alloc()`, so
`rpmsg_socket_ept_release()` does not free it and every conn detached here
leaks.
Fix: set `pend->crefs = 0` before `rpmsg_socket_destroy_ept(pend)`.
--
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]
