Closed anordal closed 2 months ago
Thanks for reporting. We'll have the team go through the code flow you mentioned above and see if any action needs to be taken
@anordal
Thanks for sharing this detailed report.
Your assessment regarding prvTCPReturnPacket_IPV4
seems to be correct when ipconfigZERO_COPY_TX_DRIVER
is enabled. The line pxIPHeader = ( ( IPHeader_t * ) &( pxNetworkBuffer->pucEthernetBuffer[ ipSIZE_OF_ETH_HEADER ] ) );
should be performed inside the if( pxNetworkBuffer != NULL )
check.
Regarding prvSendDHCPRequest
, vDHCPProcess
is called in 2 places:
prvProcessNetworkDownEvent
: NULL
pointer check is performed on pxEndPoint
before the call to vDHCPProcess
here.prvCallDHCP_RA_Handler
: This function is called when there is a eDHCPEvent
received in the IP task. DHCP event is sent from the following places:
vCheckNetworkTimers
: NULL pointer check.xProcessReceivedUDPPacket_IPv4
\ xProcessReceivedUDPPacket_IPv6
: Before processing any received ethernet packet both interface and endpoint of the packet is checked to be not NULL here.Hence I believe the 2nd case is indeed a false positive as you suggested.
Thanks!
prvTCPReturnPacket_IPV4
: Yes, I don't know why I didn't see that solution – pxIPHeader
is indeed only used in that scope – its scope can be reduced. It looks tempting to move most of the variable declarations into that scope. The single NetworkInterface_t * pxInterface;
declaration there looks so lonely ;-)
prvProcessNetworkDownEvent
: Hm, maybe you see something I don't, but note that the supposed null pointer is xDHCPSocket
– in the left side of the assignment. Here it is inside the xSocketValid check:
if( ( xSocketValid( EP_DHCPData.xDHCPSocket ) == pdTRUE ) && /* … */ )
{
// …
EP_DHCPData.xDHCPSocket->pxEndPoint = pxEndPoint;
My thinking was that it is enough to observe that xSocketValid() only returns pdTRUE if its argument is non-null.
Clang-tidy can't see into xSocketValid()
(because it's defined in a different translation unit, and clang-tidy is essentially a compiler). If I define a prvSocketValid()
in the same file that does the same, and replace the call to xSocketValid with a call to it, clang-tidy is indeed happy. It is (arguably) a compiler bug that it allows itself to make an invalid assumption and raise a false accusation when it's unable to prove something.
Will you be interested in creating a PR to fix this issue, or would you prefer that we create one?
I would be delighted! I'm writing a commit. It looks like the corresponding *_IPv6 function has the same problem. I'll check if it does and then include that too.
I can also move the variables into the scope. In a separate commit.
With merged PR, I am closing this issue. Thanks for the PR.
This applies to the latest release (V4.1.0) as well as origin/HEAD as of writing.
Clang-tidy (16 and 18) complains about a couple of possible null pointer dereferences in this repository, and I think it is right about one of them. They are of the form of a null pointer check (hinting that it can be NULL) and a dereference outside the check that dereferences it anyway.
function prvTCPReturnPacket_IPV4(), variable pxNetworkBuffer:
source/FreeRTOS_TCP_Transmission_IPv4.c:147:44: warning: Access to field 'pucEthernetBuffer' results in a dereference of a null pointer (loaded from variable 'pxNetworkBuffer'
This is the one I think clang-tidy is right about. Here, the unguarded dereference is accompanied by null pointer checks both before and after:
Interestingly, the section above this would have made sure it isn't a null pointer if it wasn't for assigning it again here. If the assignment here (in the ipconfigZERO_COPY_TX_DRIVER section) is to uphold the same guarrantee, it is missing some error handling.
function prvSendDHCPRequest(), variable EP_DHCPData.xDHCPSocket:
This must be a false positive, but I'll include it. Clang is looking across 3 funciton calls to build a case for how this pointer can be null at the place of dereference, but ignores the possibility that
xSocketValid()
can contain the crucial null pointer check, which it does.In function vDHCPProcess:
In function vDHCPProcessEndPoint:
In function prvSendDHCPRequest: