Skip to content

Commit 8306872

Browse files
hlefhlef
committed
Rework network_socket_close error paths.
49f42c1 forgot to add unlocks on error paths in `network_socket_close`. Address this by using a `LockGuard` and the new `release` API. At the same time, clarify that we have two types of errors here: those where the call can be retried (e.g., called with the wrong malloc capability, or an invalid timeout), and those which cannot be retried (the socket close failed due to an unspecified error and we went ahead to free it). Both are currently under -EAGAIN. We should differentiate the two to give a chance to the caller to call again with the right arguments. Address this by moving unrecoverable errors to -ENOTRECOVERABLE and update the documentation accordingly. Finally, a socket close return value of 0 is unrecoverable and most likely due to an internal error, so in that case we should go ahead and free everything instead of returning -EAGAIN. Signed-off-by: Hugo Lefeuvre <[email protected]>
1 parent 100f6c8 commit 8306872

File tree

2 files changed

+87
-73
lines changed

2 files changed

+87
-73
lines changed

‎include/NetAPI.h‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,15 @@ NetworkAddress __cheri_compartment("NetAPI")
102102
*
103103
* Returns 0 on success, or a negative error code on failure:
104104
*
105-
* - -EINVAL: The socket is not valid or the malloc capability does not match
106-
* the socket.
107-
* - -ETIMEDOUT: The timeout was reached before the socket could be closed.
105+
* - -EINVAL: Invalid argument (the socket is not valid, the malloc capability
106+
* does not match the socket, or the timeout is invalid). When
107+
* -EINVAL is returned, no resources were freed and the socket was
108+
* not closed. The operation can be retried with correct arguments.
109+
* - -ETIMEDOUT: The timeout was reached before the socket could be closed. No
110+
* resources were freed and the socket was not closed. The operation
111+
* can be retried.
112+
* - -ENOTRECOVERABLE: An error occurred and the socket was partially freed or
113+
* closed. The operation cannot be retried.
108114
*/
109115
int __cheri_compartment("TCPIP")
110116
network_socket_close(Timeout *t, SObj mallocCapability, SObj sealedSocket);

‎lib/tcpip/network_wrapper.cc‎

Lines changed: 78 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -558,87 +558,95 @@ int network_socket_close(Timeout *t, SObj mallocCapability, SObj sealedSocket)
558558
}
559559
return with_sealed_socket(
560560
[=](SealedSocket *socket) {
561-
// Don't use a lock guard here, we don't want to release the lock if
562-
// it's been deallocated. This must be released on all return paths
563-
// except the one for success.
564-
if (!socket->socketLock.try_lock(t))
561+
if (LockGuard g{socket->socketLock, t})
565562
{
566-
return -ETIMEDOUT;
567-
}
568-
// Since we free the socket and the token at the end after
569-
// terminating the socket, ensure that the frees won't fail
570-
if (heap_can_free(mallocCapability, socket->socket) != 0 ||
571-
token_obj_can_destroy(
572-
mallocCapability, socket_key(), sealedSocket) != 0)
573-
{
574-
return -EINVAL;
575-
}
576-
bool isTCP = socket->socket->ucProtocol == FREERTOS_IPPROTO_TCP;
577-
// Shut down the socket before closing the firewall. Don't
578-
// bother with the return value: `FreeRTOS_shutdown` fails
579-
// mainly if the TCP connection is dead, which is likely to
580-
// happen in practice and has no impact for us.
581-
FreeRTOS_shutdown(socket->socket, FREERTOS_SHUT_RDWR);
582-
auto localPort = ntohs(socket->socket->usLocalPort);
583-
if (socket->socket->bits.bIsIPv6)
584-
{
585-
if (isTCP)
563+
// Since we free the socket and the token at the end after
564+
// terminating the socket, ensure that the frees won't fail
565+
if (heap_can_free(mallocCapability, socket->socket) != 0 ||
566+
token_obj_can_destroy(
567+
mallocCapability, socket_key(), sealedSocket) != 0)
586568
{
587-
firewall_remove_tcpipv6_endpoint(localPort);
569+
Debug::log("Unable to free socket or token.");
570+
// The main reason why this would fail is because we
571+
// were called with the wrong malloc capability. We
572+
// want to leave a chance to the caller to call us
573+
// again with the right capability.
574+
return -EINVAL;
575+
}
576+
bool isTCP = socket->socket->ucProtocol == FREERTOS_IPPROTO_TCP;
577+
// Shut down the socket before closing the firewall. Don't
578+
// bother with the return value: `FreeRTOS_shutdown` fails
579+
// mainly if the TCP connection is dead, which is likely to
580+
// happen in practice and has no impact for us.
581+
FreeRTOS_shutdown(socket->socket, FREERTOS_SHUT_RDWR);
582+
auto localPort = ntohs(socket->socket->usLocalPort);
583+
if (socket->socket->bits.bIsIPv6)
584+
{
585+
if (isTCP)
586+
{
587+
firewall_remove_tcpipv6_endpoint(localPort);
588+
}
589+
else
590+
{
591+
firewall_remove_udpipv6_local_endpoint(localPort);
592+
}
588593
}
589594
else
590595
{
591-
firewall_remove_udpipv6_local_endpoint(localPort);
596+
if (isTCP)
597+
{
598+
firewall_remove_tcpipv4_endpoint(localPort);
599+
}
600+
else
601+
{
602+
firewall_remove_udpipv4_local_endpoint(localPort);
603+
}
592604
}
593-
}
594-
else
595-
{
596-
if (isTCP)
605+
// Close the socket. Another thread will actually clean up the
606+
// memory. This returns 1 on success.
607+
int ret = 0;
608+
auto closeStatus = close_socket_retry(t, socket->socket);
609+
if (closeStatus == 0)
597610
{
598-
firewall_remove_tcpipv4_endpoint(localPort);
611+
// The only reason why this would fail is internal
612+
// corruption (did someone already close the
613+
// socket?). Nothing can be done by anyone at this
614+
// stage. Don't return because we would leak
615+
// everything.
616+
ret = -ENOTRECOVERABLE;
599617
}
600-
else
618+
else if (closeStatus != 1)
601619
{
602-
firewall_remove_udpipv4_local_endpoint(localPort);
620+
// The close couldn't be delivered to the IP
621+
// task. With some luck, the socket can be
622+
// freed next time we try.
623+
return -ETIMEDOUT;
603624
}
625+
g.release();
626+
socket->socketLock.upgrade_for_destruction();
627+
// Drop the caller's claim on the socket.
628+
if (heap_free(mallocCapability, socket->socket) != 0)
629+
{
630+
// This is not supposed to happen, since we did a
631+
// `heap_can_free` earlier (unless we did not have
632+
// enough stack, or a concurrent free happened). If
633+
// it does, we may be leaking the socket.
634+
Debug::log("Failed to free socket.");
635+
// Don't return yet, try to at least free the token.
636+
ret = -ENOTRECOVERABLE;
637+
}
638+
if (token_obj_destroy(
639+
mallocCapability, socket_key(), sealedSocket) != 0)
640+
{
641+
// This is not supposed to happen, since we did a
642+
// `token_obj_can_destroy` earlier (see comment
643+
// above). If it does, we're leaking the token.
644+
Debug::log("Failed to free token.");
645+
ret = -ENOTRECOVERABLE;
646+
}
647+
return ret;
604648
}
605-
// Close the socket. Another thread will actually clean up the
606-
// memory.
607-
auto closeStatus = close_socket_retry(t, socket->socket);
608-
if (closeStatus == 0)
609-
{
610-
return -EINVAL;
611-
}
612-
if (closeStatus != 1)
613-
{
614-
// With some lock, the socket can be freed next time
615-
// we try.
616-
return -ETIMEDOUT;
617-
}
618-
socket->socketLock.upgrade_for_destruction();
619-
// Drop the caller's claim on the socket.
620-
int ret = 0;
621-
if (heap_free(mallocCapability, socket->socket) != 0)
622-
{
623-
// This is not supposed to happen, since we did a
624-
// heap_can_free earlier. If it does, we're leaking
625-
// the socket.
626-
Debug::log("Failed to free socket.");
627-
// Release the lock so that we don't leak it.
628-
socket->socketLock.unlock();
629-
// Don't return now, try to at least free the token.
630-
ret = -EINVAL;
631-
}
632-
if (token_obj_destroy(mallocCapability, socket_key(), sealedSocket) !=
633-
0)
634-
{
635-
// This is not supposed to happen, since we did a
636-
// token_obj_can_destroy earlier. If it does, we're
637-
// leaking the token.
638-
Debug::log("Failed to free token.");
639-
ret = -EINVAL;
640-
}
641-
return ret;
649+
return -ETIMEDOUT;
642650
},
643651
sealedSocket);
644652
}

0 commit comments

Comments
 (0)