Rework network_socket_close error paths.
#22
Conversation
lib/tcpip/network_wrapper.cc
Outdated
| // Don't return now, try to at least free the token. | ||
| ret = -EINVAL; | ||
| ret = -ENOTRECOVERABLE; |
There was a problem hiding this comment.
I believe the only way to reach this is to have insufficient stack to call heap_free. Is there any other way that it can happen?
There was a problem hiding this comment.
Agree. Of course there is also the possibility that the caller frees it concurrently as it owns the heap capability but this has no impact here. And there is also the possibility of a bug somewhere, but in theory it shouldn't happen.
There was a problem hiding this comment.
We can check for the former case by checking -ECOMPARTMENTFAIL on the heap_free return and the latter by checking the tag bit.
In the case where there's a concurrent free, we should continue freeing the other things.
There was a problem hiding this comment.
In the case where there's a concurrent free, we should continue freeing the other things.
This is already what we do?
We can check for the former case by checking -ECOMPARTMENTFAIL on the heap_free return and the latter by checking the tag bit.
What should we check it for exactly? To provide a more explicit error code?
lib/tcpip/network_wrapper.cc
Outdated
| // Don't return now, try to at least free the token. | ||
| ret = -EINVAL; | ||
| ret = -ENOTRECOVERABLE; |
There was a problem hiding this comment.
We can check for the former case by checking -ECOMPARTMENTFAIL on the heap_free return and the latter by checking the tag bit.
In the case where there's a concurrent free, we should continue freeing the other things.
hlef
force-pushed
the
hlefeuvre/fix-locking-close
branch
2 times, most recently
from
e314486 to
b7412d8
Compare
|
I have fixed the comments. Note that this will fail until we merge CHERIoT-Platform/cheriot-rtos#213 |
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]>
hlef
force-pushed
the
hlefeuvre/fix-locking-close
branch
from
b7412d8 to
8306872
Compare
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. I think that 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-ENOTRECOVERABLEand 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.Note: Since all errors currently under
-ENOTRECOVERABLEare internal, not supposed to happen (modulo bug), and not acted upon (we're leaking memory, but there really isn't anything we can do about that apart from maybe reseting, once we have it working?), we could also simply move them as debug asserts and ignore the failure.