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

Dev integration hein.v8 #738

Merged

Conversation

htibosch
Copy link
Contributor

@htibosch htibosch commented Feb 21, 2023

Description

All sorts of tests have been performed:

This time I tested close to 100% of the protocols and situations.

Especially DNS and the local name-service protocols ( mDNS, LMNNR, NBNS ), both with IPv4 and IPv6 addresses.

Another important change is that UDPv6 packets will only be processed after the IP-address has been seen in a "Neighbor Advertisement".

ACK number in TCP RESET reply to SYN packet

ICMP/ping: active and passive, through the Internet and locally.

TCP active/passive, Internet and LAN

● FreeRTOS_UDP_IPv6.c : xProcessReceivedUDPPacket_IPv6(): removed test on ipLOCAL_IP_ADDRESS_POINTER

● FreeRTOS_UDP_IPv6.c : Received UDPv6 packets must also wait for address resolution, just like IPv4 packets: xCheckRequiresARPResolution(). For now, this will only be done for link-local addresses. Maybe rename xCheckRequiresARPResolution() to xCheckRequiresAddressResolution()?

● FreeRTOS_TCP_Transmission_IPV6.c : prvTCPSendSpecialPktHelper_IPV6() : applied the patch for sending correct seq in TCP RST packet.

● FreeRTOS_TCP_Transmission_IPV4.c : prvTCPSendSpecialPktHelper_IPV4() : applied the patch for sending correct seq in TCP RST

● FreeRTOS_TCP_IP.c : vTCPStateChange(): made important logging look nicer: both IPv4 and IPv6 addresses now shown correctly.

● FreeRTOS_Sockets.c : vTCPNetStat_TCPSocket(): corrected a FreeRTOS_printf() format.

● FreeRTOS_Routing.c : pxEasyFit() : made some changes after much testing

● FreeRTOS_ND.c : in various functions, allow that ppxEndPoint equals NULL

+    if( ppxEndPoint != NULL )
+    {
         *( ppxEndPoint ) = pxEndPoint;
+    }

● FreeRTOS_ND.c : vNDAgeCache(): when ucAge becomes zero, stop processing the record.

● FreeRTOS_ND.c : prvCheckWaitingBuffer() : when ND sollicitation is answered by an advertisement, process the packet in pxARPWaitingNetworkBuffer and clear it.

● FreeRTOS_ND.c : When ipICMP_NEIGHBOR_ADVERTISEMENT_IPv6 is received, call prvCheckWaitingBuffer()

● FreeRTOS_IP.c : Call xCheckRequiresARPResolution() for both IPv4 and IPv6 packets, before refreshing the MAC/address table

● FreeRTOS_DNS_Parser.c : DNS_ParseDNSReply() : instead of always using ipUDP_PAYLOAD_OFFSET_IPv4 as the UDP offset, look at the packets received.

+    uxUDPOffset = ( size_t ) ( pucUDPPayloadBuffer - pxNetworkBuffer->pucEthernetBuffer );
+    configASSERT( ( uxUDPOffset == ipUDP_PAYLOAD_OFFSET_IPv4 ) || ( uxUDPOffset == ipUDP_PAYLOAD_OFFSET_IPv6 ) );

● FreeRTOS_DNS_Parser.c : prepareReplyDNSMessage() : correct ucTimeToLive for mDNS packets.

● FreeRTOS_DNS_Networking.c: added a cast in a call to FreeRTOS_printf()

● FreeRTOS_ARP.c : vARPProcessPacketReply() : see if the IP-type of pxARPWaitingNetworkBuffer is IPv4

● FreeRTOS_ARP.c : xCheckRequiresARPResolution() : let it also check IPv6 frames, and have it call vNDSendNeighbourSolicitation() when necessary.

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 February 21, 2023 12:43
@@ -0,0 +1,693 @@
/*
* FreeRTOS+TCP V2.3.1
Copy link
Member

Choose a reason for hiding this comment

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

Is this file duplicate in include folder?

vNDRefreshCacheEntry( &( pxIPPacket->xEthernetHeader.xSourceAddress ), &( pxIPHeader_IPv6->xSourceAddress ), pxNetworkBuffer->pxEndPoint );
}
else

if( xCheckRequiresARPResolution( pxNetworkBuffer ) == pdTRUE )
Copy link
Member

Choose a reason for hiding this comment

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

Why is ARP resolution needed here for IPv6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes maybe we should give that function a different name like: xCheckRequiresAddressResolution(). In this PR I added code to handle both IPv4 and IPv6 packets.

source/FreeRTOS_TCP_Transmission_IPV4.c Show resolved Hide resolved
* so increase the variable with 1. Before sending a reply, the values of
* 'ulSequenceNumber' and 'ulAckNr' will be swapped. */
uint32_t ulSequenceNumber = FreeRTOS_ntohl( pxTCPPacket->xTCPHeader.ulSequenceNumber );
ulSequenceNumber++;
Copy link
Member

Choose a reason for hiding this comment

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

Hi Hein, can you explain a little more on this, for which case do we need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you know the SYN and the FIN packets carry a sort of pseudo data byte. It isn't there, but the counterparty checks our ACK to know that the SYN/FIN has been seen. In earlier versions, we just copied SEQ to ACK in this situation.

Now we get a SYN for a non-existing port number. We respond with a RESET packet.
In the above code, ulSequenceNumber is increased. Later on, in prvTCPReturnPacket(), the two fields SEQ and ACK will be swapped, so ACK will be as expected.

It was easy to test: try to connect to a non-existing port number e.g. using puTTY.

@htibosch htibosch force-pushed the dev_integration_hein.v8 branch from f4edf9d to c111461 Compare February 23, 2023 09:00
@moninom1 moninom1 merged commit ff11a14 into FreeRTOS:dev/IPv6_integration Feb 23, 2023
tony-josi-aws pushed a commit to tony-josi-aws/FreeRTOS-Plus-TCP that referenced this pull request Feb 24, 2023
* Updating tcp utilities

* Some more change in dev_integration_hein.v8

* In FreeRTOS_DNS_Parser.c : use 'ipUDP_PAYLOAD_OFFSET_IPv4' in stead of 'ipIP_PAYLOAD_OFFSET'

* And a few more corrections

* Changes to WinPCap network interface, removed debugging code

* After applying uncrustify

* Oops, I forgot the push changes in include files.

* Now removing it, hopefully

---------

Co-authored-by: Nikhil Kamath <110539926+amazonKamath@users.noreply.github.com>
Co-authored-by: Monika Singh <108652024+moninom1@users.noreply.github.com>
tony-josi-aws added a commit that referenced this pull request Feb 24, 2023
* updating doxygen config

* fixing doxygen comments

* adding IPv6 files and fixing comments

* fix doxygen cfg and file names in comments

* wip doxygen v6 docs

* adding doxygen comments

* include RA src file to doxgendocs generation

* fix spell check issues

* Uncrustify: triggered by comment.

* fix minor build issue

* fix spell check issues

* Uncrustify: triggered by comment

* fix trailing white space

* Dev integration hein.v8 (#738)

* Updating tcp utilities

* Some more change in dev_integration_hein.v8

* In FreeRTOS_DNS_Parser.c : use 'ipUDP_PAYLOAD_OFFSET_IPv4' in stead of 'ipIP_PAYLOAD_OFFSET'

* And a few more corrections

* Changes to WinPCap network interface, removed debugging code

* After applying uncrustify

* Oops, I forgot the push changes in include files.

* Now removing it, hopefully

---------

Co-authored-by: Nikhil Kamath <110539926+amazonKamath@users.noreply.github.com>
Co-authored-by: Monika Singh <108652024+moninom1@users.noreply.github.com>

* Fix CBMC proofs for DNS (#718)

* Use CBMC XML output to enable VSCode debugger (#673)

Prior to this commit, CBMC would emit logging information in plain text
format, which does not contain information required for the CBMC VSCode
debugger. This commit makes CBMC use XML instead of plain text.

Co-authored-by: Mark Tuttle <tuttle@acm.org>

* wip

* wip DNSgetHostByName

* wip DNSgetHostByName

* fixed cbmc proof for DNS_ReadNameField

* wip DNSgetHostByName_a_harness

* Fix CBMC prooff for DNSgetHostByName

* wip fix DNSgetHostByName_a CBMC proof

* fixed cbmc target func not called issue in DNSclear

* fixed cbmc target func not called issue in DNSlookup

* fix DNSgetHostByName_a CBMC proof

* update comments

* more asserts

* fixing formatting

* updating as per review comments

* fix dns after review comments

* adding more asserts

* adds more asserts

* minor fix

* fixing comments

* fixing comments

* fixing minor issue

* fixing DNS_ReadReply() signature

* making code more consistant

* adding more  asserts

* making code more consistent

---------

Co-authored-by: Kareem Khazem <karkhaz@amazon.com>
Co-authored-by: Mark Tuttle <tuttle@acm.org>

* Uncrustify: triggered by comment

* fixing formatting

---------

Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Hein Tibosch <hein_tibosch@yahoo.es>
Co-authored-by: Nikhil Kamath <110539926+amazonKamath@users.noreply.github.com>
Co-authored-by: Monika Singh <108652024+moninom1@users.noreply.github.com>
Co-authored-by: Kareem Khazem <karkhaz@amazon.com>
Co-authored-by: Mark Tuttle <tuttle@acm.org>
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