Skip to content

Commit 6101271

Browse files
hlefdavidchisnall
authored andcommitted
Fix bugs in network_socket_receive_from.
A few things are wrong with `network_socket_receive_from`: - We claim `unclaimedBuffer` but don't unclaim it on error paths, thus leaking the buffer. - The `check_pointer` for `port` is done on `address`, likely due to a copy/paste issue. - The `if (received > 0)` before setting `buffer` is redundant as we already checked this a few lines earlier. - We don't document that the UDP packet will be dropped if an error occurs in this function (timeout, permissions, etc.). - The documentation of error codes is incomplete. Address all these issues. Signed-off-by: Hugo Lefeuvre <[email protected]>
1 parent 3358baf commit 6101271

File tree

2 files changed

+23
-15
lines changed

2 files changed

+23
-15
lines changed

include/NetAPI.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,13 @@ int __cheri_compartment("TCPIP")
180180
* The `bytesReceived` field of the result will be negative if an error
181181
* occurred. The `buffer` field will be null if no data were received.
182182
*
183+
* Note that errors occuring in this function (particularly timeout and invalid
184+
* `address` or `port` pointers) may cause UDP packets to be dropped.
185+
*
183186
* The negative values will be errno values:
184187
*
188+
* - `-ENOMEM`: The allocation quota is insufficient to hold the packet.
189+
* - `-EPERM`: The `address` and/or `port` pointers are invalid.
185190
* - `-EINVAL`: The socket is not valid.
186191
* - `-ETIMEDOUT`: The timeout was reached before data could be received.
187192
* - `-ENOTCONN`: The socket is not connected.

lib/tcpip/network_wrapper.cc

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -667,23 +667,26 @@ NetworkReceiveResult network_socket_receive_from(Timeout *timeout,
667667
FREERTOS_ZERO_COPY,
668668
&remoteAddress,
669669
&remoteAddressLength);
670-
});
671-
if (received > 0)
670+
} /* with_freertos_timeout */);
671+
if (received > 0)
672672
{
673-
ssize_t claimed = heap_claim(mallocCapability, unclaimedBuffer);
674-
Debug::log(
675-
"Claimed {} bytes for {}-byte buffer", claimed, received);
673+
Claim c{mallocCapability, unclaimedBuffer};
676674
FreeRTOS_ReleaseUDPPayloadBuffer(unclaimedBuffer);
677-
if (claimed <= 0)
675+
if (!c)
678676
{
677+
Debug::log("Failed to claim socket.");
679678
return -ENOMEM;
680679
}
680+
681+
Debug::log("Claimed {}-byte buffer", received);
681682
Capability claimedBuffer{unclaimedBuffer};
682683
claimedBuffer.bounds() = received;
684+
683685
if (heap_claim_fast(timeout, address, port) < 0)
684686
{
685687
return -ETIMEDOUT;
686688
}
689+
687690
if (address != nullptr)
688691
{
689692
if (!check_pointer<PermissionSet{Permission::Store}>(address))
@@ -705,19 +708,18 @@ NetworkReceiveResult network_socket_receive_from(Timeout *timeout,
705708
}
706709
if (port != nullptr)
707710
{
708-
if (!check_pointer<PermissionSet{Permission::Store}>(address))
711+
if (!check_pointer<PermissionSet{Permission::Store}>(port))
709712
{
710713
return -EPERM;
711714
}
712715
*port = FreeRTOS_ntohs(remoteAddress.sin_port);
713716
}
714-
if (received > 0)
715-
{
716-
buffer = claimedBuffer;
717-
return received;
718-
}
717+
718+
buffer = claimedBuffer;
719+
c.release();
720+
return received;
719721
}
720-
else if (received < 0)
722+
if (received < 0)
721723
{
722724
if (received == -pdFREERTOS_ERRNO_ENOTCONN)
723725
{
@@ -728,12 +730,13 @@ NetworkReceiveResult network_socket_receive_from(Timeout *timeout,
728730
{
729731
return -ETIMEDOUT;
730732
}
733+
731734
// Something went wrong?
732735
Debug::log("Receive failed with unexpected error: {}", received);
733736
return -EINVAL;
734737
}
735-
return received;
736-
},
738+
return received; // We had `received` == 0.
739+
} /* with_sealed_socket */,
737740
socket);
738741
return {result, buffer};
739742
}

0 commit comments

Comments
 (0)