Skip to content

Commit 7142e42

Browse files
hlefhlef
authored
Merge pull request #22 from CHERIoT-Platform/hlefeuvre/fix-locking-close
Rework `network_socket_close` error paths.
2 parents 100f6c8 + 8306872 commit 7142e42

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)