Rework network_socket_close error paths.

49f42c1 forgot to add unlocks on error
paths in `network_socket_close`. Address this.

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]>

include/NetAPI.h

@@ -102,9 +102,15 @@ NetworkAddress __cheri_compartment("NetAPI")
  *
  * Returns 0 on success, or a negative error code on failure:
  *
- *  - -EINVAL: The socket is not valid or the malloc capability does not match
- *    the socket.
- *  - -ETIMEDOUT: The timeout was reached before the socket could be closed.
+ *  - -EINVAL: Invalid argument (the socket is not valid, the malloc capability
+ *             does not match the socket, or the timeout is invalid). When
+ *             -EINVAL is returned, no resources were freed and the socket was
+ *             not closed. The operation can be retried with correct arguments.
+ *  - -ETIMEDOUT: The timeout was reached before the socket could be closed. No
+ *             resources were freed and the socket was not closed. The operation
+ *             can be retried.
+ *  - -ENOTRECOVERABLE: An error occurred and the socket was partially freed or
+ *             closed. The operation cannot be retried.
  */
 int __cheri_compartment("TCPIP")
   network_socket_close(Timeout *t, SObj mallocCapability, SObj sealedSocket);

lib/tcpip/network_wrapper.cc

@@ -558,9 +558,9 @@ int network_socket_close(Timeout *t, SObj mallocCapability, SObj sealedSocket)
 	}
 	return with_sealed_socket(
 	  [=](SealedSocket *socket) {
-		  // Don't use a lock guard here, we don't want to release the lock if
-		  // it's been deallocated.  This must be released on all return paths
-		  // except the one for success.
+		  // Don't use a lock guard here, we don't want to release the
+		  // lock if it's been deallocated.  This must be released on
+		  // all return paths except the one for success.
 		  if (!socket->socketLock.try_lock(t))
 		  {
 			  return -ETIMEDOUT;
@@ -571,6 +571,13 @@ int network_socket_close(Timeout *t, SObj mallocCapability, SObj sealedSocket)
 		      token_obj_can_destroy(
 		        mallocCapability, socket_key(), sealedSocket) != 0)
 		  {
+			  Debug::log("Unable to free socket or token.");
+			  // Release the lock so that we don't leak it.  The
+			  // main reason why this would fail is because we were
+			  // called with the wrong malloc capability. We want
+			  // to leave a chance to the caller to call us again
+			  // with the right capability.
+			  socket->socketLock.unlock();
 			  return -EINVAL;
 		  }
 		  bool isTCP = socket->socket->ucProtocol == FREERTOS_IPPROTO_TCP;
@@ -604,30 +611,34 @@ int network_socket_close(Timeout *t, SObj mallocCapability, SObj sealedSocket)
 		  }
 		  // Close the socket.  Another thread will actually clean up the
 		  // memory.
+		  int  ret         = 0;
 		  auto closeStatus = close_socket_retry(t, socket->socket);
 		  if (closeStatus == 0)
 		  {
-			  return -EINVAL;
+			  // The only reason why this would fail is internal
+			  // corruption (did someone already close the
+			  // socket?). Nothing can be done by anyone at this
+			  // stage. Don't return because we would leak
+			  // everything.
+			  ret = -ENOTRECOVERABLE;
 		  }
 		  if (closeStatus != 1)
 		  {
-			  // With some lock, the socket can be freed next time
+			  socket->socketLock.unlock();
+			  // With some luck, the socket can be freed next time
 			  // we try.
 			  return -ETIMEDOUT;
 		  }
 		  socket->socketLock.upgrade_for_destruction();
 		  // Drop the caller's claim on the socket.
-		  int ret = 0;
 		  if (heap_free(mallocCapability, socket->socket) != 0)
 		  {
 			  // This is not supposed to happen, since we did a
 			  // heap_can_free earlier. If it does, we're leaking
 			  // the socket.
 			  Debug::log("Failed to free socket.");
-			  // Release the lock so that we don't leak it.
-			  socket->socketLock.unlock();
 			  // Don't return now, try to at least free the token.
-			  ret = -EINVAL;
+			  ret = -ENOTRECOVERABLE;
 		  }
 		  if (token_obj_destroy(mallocCapability, socket_key(), sealedSocket) !=
 		      0)
@@ -636,7 +647,7 @@ int network_socket_close(Timeout *t, SObj mallocCapability, SObj sealedSocket)
 			  // token_obj_can_destroy earlier. If it does, we're
 			  // leaking the token.
 			  Debug::log("Failed to free token.");
-			  ret = -EINVAL;
+			  ret = -ENOTRECOVERABLE;
 		  }
 		  return ret;
 	  },