FreeRTOS / FreeRTOS-Plus-TCP

FreeRTOS-Plus-TCP library repository. +TCP files only. Submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
MIT License
152 stars 163 forks source link

[BUG] xDNS_IP_Preference gets set to xPreferenceIPv6 and forgotten #1107

Closed evpopov closed 8 months ago

evpopov commented 9 months ago

The default value of xDNS_IP_Preference is xPreferenceIPv4, however if I ever query and IPv6 address, +TCP sets xDNS_IP_Preference to xPreferenceIPv6 and from that point on, all name resolutions happen over IPv6

To reproduce, use the following in your FreeRTOSIPConfig.h

#define ipconfigUSE_LLMNR                   ( 1 )
#define ipconfigUSE_NBNS                    ( 0 )
#define ipconfigUSE_MDNS                    ( 1 )

Here's my test code:

static void NameResolutionTestTask( void *pvParameter)
{
    pHostname = "idu21h13aa24";    // <-- Test using LLMNR by default
    // pHostname = "idu21h13aa24.local";    // <-- Same result but using mDNS
    struct freertos_addrinfo xHints;
    struct freertos_addrinfo * pxResult;
    uint8_t * pHostname;
    memset( &xHints, 0x00, sizeof( xHints ) );

    vTaskDelay( pdMS_TO_TICKS(5000) );

    for ( int i = 0; true; i++)
    {
        vTaskDelay(5000);
        if ( i & 1)
        {
            xHints.ai_family = FREERTOS_AF_INET6;
        }
        else
        {
            xHints.ai_family = FREERTOS_AF_INET4;
        }

        BaseType_t xGetAddrInfoResult = FreeRTOS_getaddrinfo_a( pHostname, NULL, &xHints, &pxResult, prvResolve_Callback, i, pdMS_TO_TICKS(500) );
        FreeRTOS_freeaddrinfo( pxResult );
    }

}

and here's the resulting packet capture: v4v6_Preference.zip

I can see that the above is caused by the code in prvGetHostByNameOp() but I'm not sure what the original intent was.

My actual use case involves one of my devices trying to connect to another by looking up it's IP over LLMNR or mDNS. I want to resolve the name to hopefully an IPv6 address, but if the target device has old firmware, it does not support IPv6 and I want the option of resolving it's IPv4 address. That is the reason I have to call prvGetHostByNameOp() with both v4 and v6 hints. With the current behavior, if the "older" device misses the initial IPv4 resolution attempt, it will never respond because it only supports IPv4.

I did a quick and dirty test by replacing

        #if ( ipconfigUSE_IPv6 != 0 )
            if( xFamily == ( BaseType_t ) FREERTOS_AF_INET6 )
            {
                xDNS_IP_Preference = xPreferenceIPv6;
            }
        #endif /* ( ipconfigUSE_IPv6 != 0 ) */

with

        #if ( ipconfigUSE_IPv6 != 0 )
            if( xFamily == ( BaseType_t ) FREERTOS_AF_INET6 )
            {
                xDNS_IP_Preference = xPreferenceIPv6;
            }
        else
            {
                xDNS_IP_Preference = xPreferenceIPv4;
            }
        #endif /* ( ipconfigUSE_IPv6 != 0 ) */

That seems to fix the issue for my exact use case so IPv4 addresses get resolved over IPv4 and IPv6 addresses get resolver over IPv6. Again, I'm not exactly sure what the intent was, so I don't want to rush to solutions that haven't been thought through.

Another thing I noticed is that xPreferenceNone isn't used anywhere. Isn't that supposed to be the default that maybe refers causes IPv4 queries? Lastly, I have only done testing with LLMNR and mDNS. I have not tested operation of the the actual DNS queries with a proper IPv4/IPv6 DNS server.

moninom1 commented 8 months ago

Hi @evpopov Thank you for reporting the issue, from the first glance it seems like NameResolutionTestTask should take care of both v4 and v6 case, but will look further into it and update.

htibosch commented 8 months ago

@evpopov wrote:

Again, I'm not exactly sure what the intent was, so I don't want to rush to solutions that haven't been thought through.

The variable xDNS_IP_Preference is there only for testing purposes, so it is normal that it never changes.

When I test DNS lookups, I can either type "dnsq4 site.com" or "dnsq6 site.com". xDNS_IP_Preference will be set to either xPreferenceIPv4 or xPreferenceIPv6, depending on the command.

And yes it worked all right for me, I tested it very often.

Mind the 4 combinations: contact DNS using UDPv4 or UDPv6, and ask for either A or AAAA records,

evpopov commented 8 months ago

@htibosch Thanks for the input about xDNS_IP_Preference. As I said, I haven't been able to test proper DNS. I only tested LLMNR and mDNS. Maybe they worked as expected in the past, but that functionality must have broken somewhere along the way because my Wireshark capture clearly shows that even though the test task alternates between xPreferenceIPv4 and xPreferenceIPv6 only the very first query is over IPv4.

I am aware of the 4 combinations but I don't see any control over all 4. The user calling FreeRTOS_getaddrinfo_a() only gives one hint so they can't control all 4 combinations unless we add more enum values. With that being said, I personally don't see tremendous need for the "cross combinations": A record over IPv6 or AAAA record over IPv6. Those queries are cool, but I don't consider them a requirement. Maybe they are useful for the true DNS case.... I don't know. +TCP is an embedded stack and doesn't need to have all the bells and whistles so I think the current behavior is adequate. The user is free to use one preference and then fall-back to the other if they don't get a reply. That type of behavior would certainly solve my issue where I want to resolve get an AAAA record and optionally fall-back to A record over IPv4 of the remote device simply doesn't support IPv6.

moninom1 commented 8 months ago

Thank you all for the valuable inputs, we have merged PR1109 as a fix, so will close the issue for now. Feel free to reopen if something is missed.