FreeRTOS+TCP : pass payload length when calling UDP callback

[htibosch]

Description

Recently, it was decided to let NetworkBufferDescriptor::xDataLength always mean: the total size of the network packet. Before that, it could also mean: "payload size", depending on the location in the code. That was confusing.
This was changed in PR #1513.

However, by mistake, I hadn't tested the ipconfigUSE_CALLBACKS option with UDP after the PR.

In xProcessReceivedUDPPacket(), the value of xDataLength should have been corrected with ipUDP_PAYLOAD_OFFSET_IPv4:

+    /* The value of 'xDataLength' was proven to be at least the size of a
+    UDP packet in prvProcessIPPacket(). */
     if( xHandler( ( Socket_t ) pxSocket,
                   ( void* ) pcData,
-                  ( size_t ) pxNetworkBuffer->xDataLength,
+                  ( size_t ) ( pxNetworkBuffer->xDataLength - ipUDP_PAYLOAD_OFFSET_IPv4 ),
                   &xSourceAddress, &destinationAddress ) )

pcData was pointing correctly to the UDP payload, but xDataLength was too large.

Checklist:

• I have tested my changes. No regression in existing tests.
• My code is Linted.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[htibosch]

In FreeRTOIPConfig.h, I added:

/* TX checksum offloading has NOT been implemented in the Wi-Fi of ESP32. */
#define ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM     0

That is redundant, because 0 is already the default. I added it for clarity, to confirm that the WiFi module can only offload checksums during reception, not during transmission.

Correction : the above comment has nothing to do with this PR 1625,
It should have been placed in PR 1626 ( Espressif ESP32 NetworkInterface ).

[gkwicker]

See comments and check unit test for possible failures after this change.

[gkwicker]

FYI we recently found an integer overflow case in the unit test for ulDNSHandlePacket() and fixed it by explicitly checking that pxNetworkBuffer->xDataLength >= sizeof( UDPPacket_t ). Will we see another failing unit test if we don't do that check prior to line 284 here?

[htibosch]

Sorry, I hadn't see your earlier PR. Thanks for pointing out. I will respond directly to it.
Thanks, Hein

[dcgaws]

I'm confused by the implication of the change in behavior from the perspective of the callee. Is this within the specification of xHandler?

[aggarg]

@htibosch As Gary pointed out do we need to make sure that pxNetworkBuffer->xDataLength >= ipUDP_PAYLOAD_OFFSET_IPv4 before doing pxNetworkBuffer->xDataLength - ipUDP_PAYLOAD_OFFSET_IPv4 i.e.

if( pxNetworkBuffer->xDataLength >= ipUDP_PAYLOAD_OFFSET_IPv4 )
{
    /* Call xHandler. */
}
else
{
    /* Handle malformed packet. */
}

[gkwicker]

@aggarg, @htibosch 's comment on PR #1615 points out that this was validated in prvProcessIPPacket(), so we are probably okay here. That PR was in response to a failing unit test in which ulDNSHandlePacket() was called directly, so that condition hadn't been guaranteed. We'll move forward with this and monitor the unit test results for any regressions.

[dcgaws]

I'm confused by the implication of the change in behavior from the perspective of the callee. Is this within the specification of xHandler?

[htibosch]

@dcgaws wrote:

I'm confused by the implication of the change in behavior from the
perspective of the callee. Is this within the specification of xHandler?

Yes it is. My xHandler was complaining because the packets were too long since applying PR 1625.
The application hook indeed expects a pointer to the payload, and the length of the payload.

[gkwicker]

@aggarg, @htibosch 's comment on PR #1615 points out that this was validated in prvProcessIPPacket(), so we are probably okay here. That PR was in response to a failing unit test in which ulDNSHandlePacket() was called directly, so that condition hadn't been guaranteed. We'll move forward with this and monitor the unit test results for any regressions.