Improves handling of IPv4 broadcast addresses. #1221
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add xIsIPv4Broadcast helper function that enumerates over all IPv4 endpoints and their corresponding broadcast addresses. Adds checks for the 255.255.255.255 address. Substitutes ipBROADCAST_IP_ADDRESS with FREERTOS_INADDR_BROADCAST to better match the rest of the world. Cleans up frame filtering a bit to ensure a frame's destination address is NOT matched against other endpoints than the one the frame entered through.
Contributor
Author
|
/bot run formatting |
source/FreeRTOS_IPv4.c
Outdated
| struct xNetworkEndPoint ** ppxEndPoint ) | ||
| { | ||
| BaseType_t xIsBroadcast; | ||
| uint32_t ulEndPointBroadcast; |
Member
There was a problem hiding this comment.
NIT: Unused variable ulEndPointBroadcast
| uint16_t usProtocolBytes; /**< The total length of the protocol data. */ | ||
| }; | ||
|
|
||
| #define ipBROADCAST_IP_ADDRESS 0xffffffffU |
Member
There was a problem hiding this comment.
There are few more references to ipBROADCAST_IP_ADDRESS in the code - STM32 network interfaces and UTs
Member
|
Thanks @evpopov for this PR, I have added some minor comments. |
evpopov
force-pushed
the
BCastAddressDetection_PR
branch
from
9955e6d to
7f7c4cc
Compare
Removes an unused variable Renames ipBROADCAST_IP_ADDRESS to FREERTOS_INADDR_BROADCAST in the unit tests and the STM32 network driver
evpopov
force-pushed
the
BCastAddressDetection_PR
branch
from
7f7c4cc to
fcc0bde
Compare
Contributor
Author
|
Thanks for catching those @tony-josi-aws |
tony-josi-aws
added
enhancement
tony-josi-aws
approved these changes
Feb 6, 2025
kar-rahul-aws
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The current code incorrectly assumes that broadcast addresses only end in x.x.x.255
When a frame enters the stack, all broadcast tests must be made against the 255.255.255.255 address and against the specific broadcast of the endpoint that the frame was assigned
When an ARP lookup is attempted, we need to check if the user is attempting to send a packet that may be considered a broadcast on any of our IPv4 endpoints. Current code was only calling an outgoing packet a "broadcast" is said packet ended in 0xFF.
This PR attempts to correct both of the above.
Additionally, I've taken the liberty to substitute ipBROADCAST_IP_ADDRESS with FREERTOS_INADDR_BROADCAST and put the new definition in a public header file. Here's my reasoning:
I have also probably broken a bunch of test cases with this and I'd really appreciate it if someone can help with fixing the old tests and maybe even add tests that verify the new changes. For example test that verifies 192.168.0.7 as a broadcast if the endpoint is 192.168.0.1/29, etc...
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.