Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not clear pxEndPoint in TCPReturnPacket() #1078

Merged

Conversation

htibosch
Copy link
Contributor

@htibosch htibosch commented Jan 22, 2024

Description

In response to issue #1077 :

@XIAO-Q7, thank you for reporting the issue and for the research that you did. Also thanks to @evpopov who encountered the same problem.

I managed to reproduce the problem if I use the following configuration:

  • DUT has IP-address 192.168.2.127
  • DUT has no gateway (0.0.0.0)
  • The TCP remote party has IP address 172.16.0.1

DUT tries to connect to remote device, but eARPGetCacheEntry() clears the endpoint, which leads to a dereference of NULL.

Here is a summary of the proposed change:

-     eResult = eARPGetCacheEntry( &ulDestinationIPAddress, &xMACAddress,
-                                  &( pxNetworkBuffer->pxEndPoint ) );
+     eResult = eARPGetCacheEntry( &ulDestinationIPAddress, &xMACAddress,
+                                  &pxEndPoint );

     if( eResult == eARPCacheHit )
     {
         pvCopySource = &xMACAddress;
+        pxNetworkBuffer->pxEndPoint = pxEndPoint;
     }
     else
     {
         pvCopySource = &pxEthernetHeader->xSourceAddress;
     }

+    if( pxNetworkBuffer->pxEndPoint == NULL )
+    {
+        break;
+    }

I hadn't had the time to check the code coverage yet.

Test Steps

Use a similar configuration and try to connect to the remote device.

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

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

@htibosch htibosch requested a review from a team as a code owner January 22, 2024 09:46
@tony-josi-aws
Copy link
Member

/bot run formatting

@tony-josi-aws tony-josi-aws force-pushed the dont_clear_pxEndPoint_in_TCPReturnPacket branch from 021212f to 3133530 Compare January 25, 2024 06:28
}
else
{
pvCopySource = &pxEthernetHeader->xSourceAddress;
}

if( pxNetworkBuffer->pxEndPoint == NULL )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I had some confusion whether eARPGetCacheEntry will return a eARPCacheHit or eARPCacheMiss in this schenario and if the check (pxNetworkBuffer->pxEndPoint == NULL ) needs to be inside the return value check or outside as it is now. However, on some more exploration and internal discussions, it is clear that eARPGetCacheEntry will return eARPCacheMiss and the check can be outside and it will not have any other side-effect. With this I am good with the change.

@tony-josi-aws tony-josi-aws merged commit 0cb2d75 into FreeRTOS:main Jan 25, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants