Support caching more than 1 IP address per DNS cache entry #1981

gkwicker · 2020-05-06T17:19:39Z

Description

These changes address this FreeRTOS+TCP issue, add a test for the new multiple IP address DNS feature, and re-enable DNS caching on all FreeRTOS+TCP platforms. If FreeRTOS+TCP is configured with (ipconfigUSE_DNS_CACHE == 1) && (ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY > 1), FreeRTOS+TCP will store multiple answers from a DNS query response; successive calls to FreeRTOS_gethostbyname() will return the next answer in the list.

Configuring ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY > 1 when the DNS cache is enabled improves reliability in systems which contact load-balanced services, since addresses in the answer list can be stale. This change updates the build configurations for all FreeRTOS+TCP platforms to store up to 6 IP addresses per DNS cache entry.

Testing

Tested with Full_TCP and MQTT_System test groups on the following boards (all passing):

pc.windows
infineon.xmc4800_iotkit
infineon.xmc4800_plus_optiga_trust_x
nuvoton.numaker_iot_m487_wifi
marvell.mw320
microchip.curiosity_pic32mzef

Checklist:

[ X ] 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.

- temporarily enable DNS debug logging

- improve test output

markrtuttle · 2020-05-06T18:12:03Z

Feel free to contact me if you want help with those broken CBMC proofs.

AniruddhaKanhere · 2020-05-07T17:21:14Z

@gkwicker "Harden the code to handle uninitialized/corrupted DNS cache fields." This is nice 👍

gkwicker · 2020-05-07T21:40:57Z

/bot run checks

libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c

htibosch · 2020-05-08T03:34:34Z

libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c

+                        ulIPAddressIndex = ( xDNSCache[ x ].ucCurrentIPAddress %
+                                        xDNSCache[ x ].ucNumIPAddresses ) % ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY;
+
+                        xDNSCache[ x ].ucCurrentIPAddress++;


Suppose that 10 addresses were found. When calling this function repeatedly, I would see the addresses 0..9, 0..9, and when ucCurrentIPAddress becomes 255, I would see 0..5, followed by 0..9 again?

Why not:

if( ++xDNSCache[ x ].ucCurrentIPAddress == xDNSCache[ x ].ucNumIPAddresses ) { xDNSCache[ x ].ucCurrentIPAddress = 0; }

Making ucCurrentIPAddress a uint8_t is no problem, DNS can not return more than +/- 27 answers.

One more general question about the change:
Suppose I'm looking up the addresses of some host and it receives 5 IP-addresses.
I will call FreeRTOS_gethostbyname() repeatedly to see all addresses.
How do I know when I've seen all IP-addresses? Should I remember the first IP-address received?
Why not return 0 after the last IP-address was returned? And then the first IP-address again?

Applications using FreeRTOS_gethostbyname() should interpret the result as an independent scalar value rather than a member of a list, and they should not prefer any value over any other. When we have an implementation of FreeRTOS_getaddrinfo(), applications will have the option of choosing a particular address, which could be useful in some circumstances.

The reasoning behind this change is to give app developers an easy way to increase reliability of their app when connecting to a load-balanced service, without requiring them to change any application logic. As just one example, the amazon-freertos repo has dozens of tests and demos which can benefit immediately without any modification.

Suppose that 10 addresses were found. When calling this function repeatedly, I would see the addresses 0..9, 0..9, and when ucCurrentIPAddress becomes 255, I would see 0..5, followed by 0..9 again?

Why not:

if( ++xDNSCache[ x ].ucCurrentIPAddress == xDNSCache[ x ].ucNumIPAddresses ) { xDNSCache[ x ].ucCurrentIPAddress = 0; }

Making ucCurrentIPAddress a uint8_t is no problem, DNS can not return more than +/- 27 answers.

I didn't choose this option because I didn't want to have to do anything for thread safety for cases where two tasks both call FreeRTOS_gethostbyname(). In the event that there was a context switch after the if but before the statement in {...}, we could have a TOCTOU error resulting in an out-of-bound array access.

The side effect of doing the modulo approach as you have pointed out is that you the order that addresses appear may not reflect how they are stored internally, but this isn't important since the caller should make no assumption about these values. The only thing that is promised is that FreeRTOS_gethostbyname() will return an answer if one is available.

I didn't choose this option because I didn't want to have to do anything for thread safety for cases where two tasks both call FreeRTOS_gethostbyname(). In the event that there was a context switch after the if but before the statement in {...}, we could have a TOCTOU error resulting in an out-of-bound array access.

What if we do

if( ++xDNSCache[ x ].ucCurrentIPAddress >= xDNSCache[ x ].ucNumIPAddresses ) { /* Use >= instead of == */ .... }

Would it remove TOCTOU error? I could not think of a case where it fails. Do correct me if I am wrong.
I am just trying to reduce computation as much as we can. Modulo operation is quite expensive than a comparison statement in a lot of architectures.

Remember that if interrupts aren't masked, a context switch can happen at any time. And since this code is not protected by any mutual-exclusion mechanism such as a mutex or semaphore (and we don't want to add those if we don't have to), you can still have the error.

In any case, the code as written is simpler and avoids any possibility of this occurring.

gkwicker · 2020-05-09T17:04:44Z

/bot run checks

AniruddhaKanhere · 2020-05-11T20:56:59Z

libraries/abstractions/secure_sockets/test/iot_test_tcp.c

@@ -805,6 +805,7 @@ TEST_GROUP_RUNNER( Full_TCP )
    RUN_TEST_CASE( Full_TCP, AFQP_SOCKETS_Recv_Invalid );
    RUN_TEST_CASE( Full_TCP, AFQP_SOCKETS_htons_HappyCase );
    RUN_TEST_CASE( Full_TCP, AFQP_SOCKETS_inet_addr_quick_HappyCase );
+    RUN_TEST_CASE( Full_TCP, test_dns_cache_multiple_addresses );


Is this the correct place for this test? Since it doesn't relate to secure sockets. This is not blocking but more of a comment.

This test suite is our "networking" test suite, so it's the best place for it. I think you're right though, ideally we would have a set of layered networking test suites. But that's another set of changes.

AniruddhaKanhere · 2020-05-11T21:30:24Z

libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c

+                        ulIPAddressIndex = ( xDNSCache[ x ].ucCurrentIPAddress %
+                                        xDNSCache[ x ].ucNumIPAddresses ) % ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY;
+
+                        xDNSCache[ x ].ucCurrentIPAddress++;


I didn't choose this option because I didn't want to have to do anything for thread safety for cases where two tasks both call FreeRTOS_gethostbyname(). In the event that there was a context switch after the if but before the statement in {...}, we could have a TOCTOU error resulting in an out-of-bound array access.

What if we do

if( ++xDNSCache[ x ].ucCurrentIPAddress >= xDNSCache[ x ].ucNumIPAddresses ) { /* Use >= instead of == */ .... }

Would it remove TOCTOU error? I could not think of a case where it fails. Do correct me if I am wrong.
I am just trying to reduce computation as much as we can. Modulo operation is quite expensive than a comparison statement in a lot of architectures.

AniruddhaKanhere · 2020-05-11T21:34:12Z

vendors/cypress/boards/CYW943907AEVAL1F/aws_demos/config_files/FreeRTOSIPConfig.h

 */
-#define ipconfigUSE_DNS_CACHE                      ( 0 )
+#define ipconfigUSE_DNS_CACHE                      ( 1 )
+#define ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY      ( 6 )


Can we make this 6U? MISRA tends to dislike operations between signed and unsigned values. (Such as modulo operation in FreeRTOS_DNS.c)

This is vendor code and it likely has the same issues throughout (see line 77). If Coverity raises a warning, we'll address that in a separate PR.

AniruddhaKanhere · 2020-05-11T21:35:16Z

vendors/cypress/boards/CYW943907AEVAL1F/aws_tests/config_files/FreeRTOSIPConfig.h

 */
-#define ipconfigUSE_DNS_CACHE                      ( 0 )
+#define ipconfigUSE_DNS_CACHE                      ( 1 )
+#define ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY      ( 6 )


Can we make this 6U? MISRA tends to dislike arithmetic operations between signed and unsigned variables,.

See previous comment, that will be addressed in a PR if Coverity raises a warning.

AniruddhaKanhere · 2020-05-11T21:36:04Z

vendors/cypress/boards/CYW954907AEVAL1F/aws_demos/config_files/FreeRTOSIPConfig.h

 */
-#define ipconfigUSE_DNS_CACHE                      ( 0 )
+#define ipconfigUSE_DNS_CACHE                      ( 1 )
+#define ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY      ( 6 )


Can we make this 6U? MISRA tends to dislike arithmetic operations between signed and unsigned variables.

Correction in other ports as well if possible.

See previous comment.

yanjos-dev · 2020-05-11T22:15:07Z

vendors/cypress/boards/CYW943907AEVAL1F/aws_demos/config_files/FreeRTOSIPConfig.h

- * address is out of date, no connections to the host will succeed until the TTL
- * expires.  See this GitHub issue for more details.
- *
- * https://github.com/FreeRTOS/FreeRTOS/issues/58
 */


Do we want to have a similar note to this for a scenario where ipconfigUSE_DNS_CACHE == 1 and ipconfigDNS_CACHE_ADDRESSES_PER_ENTRY == 1? This seems like it would have the same issue that this note addresses.

It would, that's a good point. I'll add a note in the example FreeRTOSIPConfig.h about this.

Added a clarifying comment in FreeRTOSIPConfigDefaults.h

aggarg · 2020-05-13T22:04:15Z

libraries/abstractions/secure_sockets/test/iot_test_tcp.c

@@ -2961,6 +2962,61 @@ TEST( Full_TCP, AFQP_SOCKETS_htons_HappyCase )
 }
 /*-----------------------------------------------------------*/

+TEST( Full_TCP, test_dns_cache_multiple_addresses )


For consistency, can we name the test AFQP_SECURE_SOCKETS_TestDnsCacheMultipleAddresses?

The AFQP standards for Amazon FreeRTOS Qualification Program, and this test won't be added to that test suite now since that would be a qualification change. I think it would make sense to add it to that test suite eventually, however this would have to be coordinated with that program. We would change the name to an AFQP name at that time.

That makes sense - however can we use camel casing instead of snake case?

aggarg · 2020-05-13T22:11:50Z

libraries/abstractions/secure_sockets/test/iot_test_tcp.c

@@ -2961,6 +2962,61 @@ TEST( Full_TCP, AFQP_SOCKETS_htons_HappyCase )
 }
 /*-----------------------------------------------------------*/

+TEST( Full_TCP, test_dns_cache_multiple_addresses )


Is this test verifying DNS caching? The implementation of SOCKETS_GetHostByName may not use DNS caching and instead rely on DNS round robin- the test would still pass?

Yes, that's a good point. This wouldn't be the case in the FreeRTOS+TCP implementation, but it could be in others. This test will be disabled in all platforms which use non-FreeRTOS+TCP networking until we can verify that it works on them. In fact, having the word cache in this test name may be misleading, since it's not verifying that a DNS cache is enabled (I'm not sure if this would even be possible in this test), only that different IP addresses are returned for different invocations of SOCKETS_GetHostByName(). I will rename the test to something more accurate, perhaps test_dns_multiple_addresses.

aggarg · 2020-05-13T22:13:17Z

libraries/abstractions/secure_sockets/test/iot_test_tcp.c

+    /*
+     * Resolve the broker endpoint to an array of IP addresses. Each
+     * subsequent call will return one of the addresses which the
+     * name resolves to.  Call once more than the maximum number of addresses


Call once more than the maximum number of addresses per cache entry to ensure that wraparound works.

How is this ensured?

This comment is misleading for a couple of reasons, I will change it. First, since this is Secure Sockets API-level test, it has no idea what sort of DNS caching strategy is employed (if any as pointed out in a previous comment), how many addresses could be stored per entry, etc. The only thing it needs to ensure is that subsequent calls to SOCKETS_GetHostByName() on an AWS IoT endpoint return different IP addresses.

Wraparound testing in the FreeRTOS+TCP implementation of FreeRTOS_gethostbyname() needs to be handled by a unit test in that code, not a system-level test here.

aggarg · 2020-05-13T22:14:31Z

libraries/abstractions/secure_sockets/test/iot_test_tcp.c

+                ulUnique = 0;
+            }
+        }
+        if( ( ulUnique == 1 ) && ( ulNumUniqueIPAddresses < 10 ) )


Why are we trying to obtain 10 different addresses while getting only 4 is required to pass the test?

Good point, those numbers should match – will change.

* Support multiple IP addresses per DNS cache entry. * Add test for multiple IP address DNS results from AWS IoT Core endpoint. * Re-enable DNS cache on FreeRTOS+TCP platforms * Configure FreeRTOS+TCP platforms to remember up to 6 DNS answers per cache entry More information at: FreeRTOS/FreeRTOS#58

gkwicker added 16 commits May 4, 2020 10:25

Support multiple IP addresses per DNS cache entry.

e9f59e6

Add test for multiple IP address DNS cache

bdc8a7e

Run DNS multiple IP address test on all systems

1c5c5e0

Fix build failures

efefc8f

Enable debug printing

3333db7

DNS test just retrieving addresses

c00c26d

Add delay between gethostbyname() calls

8a6af51

increase number of loops in test to determine pattern

75d55fe

Allow 60 seconds to collect all DNS answers

aff590a

fix build error

a08fe63

Do not update duplicate IP addresses in DNS cache

fb54b45

- temporarily enable DNS debug logging

Eliminate duplicate scan in DNS multiple address cache

1a5025c

- improve test output

Change debug print in DNS test

78f4ead

Fix test bug, increase debug logging on nuvoton.

ef30a6c

Increase DNS response collection time to 60s

8f298fd

Merge branch 'master' of https://github.com/aws/amazon-freertos

770ad8a

gkwicker mentioned this pull request May 6, 2020

FreeRTOS+TCP should cache more than one DNS result IP address FreeRTOS/FreeRTOS#58

Closed

gkwicker added 3 commits May 6, 2020 15:47

Correct type promotion error uncovered by CBMC.

73abe57

Convert IP address index struct members to uint32_t

6ed9398

Harden the code to handle uninitialized/corrupted DNS cache fields.

8876e93

gkwicker added 8 commits May 7, 2020 10:27

Support multiple IP addresses per DNS cache entry.

39028b5

Add test for multiple IP address DNS cache

10e168b

Run DNS multiple IP address test on all systems

d9cd0ef

Fix build failures

3faa9ee

Enable debug printing

1d56c77

DNS test just retrieving addresses

14d0cbb

Add delay between gethostbyname() calls

0fea8a9

increase number of loops in test to determine pattern

ac6172e

gkwicker added 5 commits May 7, 2020 10:27

Convert IP address index struct members to uint32_t

b794e20

Harden the code to handle uninitialized/corrupted DNS cache fields.

5f2ba3a

Merge branch 'master' of https://github.com/gkwicker/amazon-freertos

d2a4bb0

Test change

1597707

Merge branch 'master' of https://github.com/aws/amazon-freertos

c636a40

htibosch reviewed May 8, 2020

View reviewed changes

gkwicker added 2 commits May 8, 2020 16:27

Address PR comments.

8c612a6

Merge branch 'master' of https://github.com/aws/amazon-freertos

f41125c

AniruddhaKanhere suggested changes May 11, 2020

View reviewed changes

yanjos-dev reviewed May 11, 2020

View reviewed changes

AniruddhaKanhere previously approved these changes May 12, 2020

View reviewed changes

gkwicker added 2 commits May 12, 2020 12:40

Merge branch 'master' of https://github.com/aws/amazon-freertos

48ebb12

Address PR comments

5414fbd

gkwicker dismissed AniruddhaKanhere’s stale review via 5414fbd May 12, 2020 20:35

Fix spelling error

5ff6831

yanjos-dev previously approved these changes May 12, 2020

View reviewed changes

AniruddhaKanhere previously approved these changes May 12, 2020

View reviewed changes

aggarg reviewed May 13, 2020

View reviewed changes

Address code review comments.

23a4ca2

gkwicker dismissed stale reviews from AniruddhaKanhere and yanjos-dev via 23a4ca2 May 13, 2020 23:16

Fix build error.

5f4b529

AniruddhaKanhere approved these changes May 13, 2020

View reviewed changes

aggarg approved these changes May 14, 2020

View reviewed changes

gkwicker merged commit bb3c7e6 into aws:master May 14, 2020

AniruddhaKanhere mentioned this pull request Jun 2, 2020

Add Project for running integration tests v2 FreeRTOS/FreeRTOS#80

Merged

AniruddhaKanhere mentioned this pull request Jul 22, 2020

Sync with +TCP amazon-FreeRTOS FreeRTOS/FreeRTOS#158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support caching more than 1 IP address per DNS cache entry #1981

Support caching more than 1 IP address per DNS cache entry #1981

gkwicker commented May 6, 2020

markrtuttle commented May 6, 2020

AniruddhaKanhere commented May 7, 2020

gkwicker commented May 7, 2020

htibosch May 8, 2020

htibosch May 8, 2020

gkwicker May 8, 2020

gkwicker May 8, 2020

AniruddhaKanhere May 11, 2020

gkwicker May 11, 2020

gkwicker commented May 9, 2020

AniruddhaKanhere May 11, 2020

gkwicker May 11, 2020

AniruddhaKanhere May 11, 2020

AniruddhaKanhere May 11, 2020

gkwicker May 11, 2020 •

edited

Loading

AniruddhaKanhere May 11, 2020

gkwicker May 11, 2020 •

edited

Loading

AniruddhaKanhere May 11, 2020

AniruddhaKanhere May 11, 2020

gkwicker May 11, 2020

yanjos-dev May 11, 2020

gkwicker May 11, 2020

gkwicker May 12, 2020

aggarg May 13, 2020

gkwicker May 13, 2020

aggarg May 13, 2020

aggarg May 13, 2020

gkwicker May 13, 2020

aggarg May 13, 2020

gkwicker May 13, 2020

aggarg May 13, 2020

gkwicker May 13, 2020

Support caching more than 1 IP address per DNS cache entry #1981

Support caching more than 1 IP address per DNS cache entry #1981

Conversation

gkwicker commented May 6, 2020

Description

Testing

Checklist:

markrtuttle commented May 6, 2020

AniruddhaKanhere commented May 7, 2020

gkwicker commented May 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkwicker commented May 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkwicker May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkwicker May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkwicker May 11, 2020 •

edited

Loading

gkwicker May 11, 2020 •

edited

Loading