::hostByName() wrongly returns OK in case of DNS error on a queued request

[barbudor]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• [no] I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Platform

• Hardware: ESP-12
• Core Version: 2.7.4.9 - Tasmota fork
• Development Env: Platformio
• Operating System: Windows

Settings in IDE

• Module: Nodemcu
• Flash Mode: dout
• Flash Size: 4MB
• lwip Variant: LWIP2_HIGHER_BANDWIDTH_LOW_FLASH
• Reset Method: nodemcu
• Flash Frequency: 40Mhz
• CPU Frequency: 80Mhz
• Upload Using: OTA
• Upload Speed: OTA

Problem Description

I found cases where ESP8266WiFiGenericClass::hostByName() returns OK (1) when a queued DNS request fails (error, timeout or unknown host). This makes the calling code believe the request well and use the default value of 255.255.255.255 as a valid destination address. When using UDP this silently translate into broadcasting of messages to the network.

Note that this have been identified within Tasmota only using the forked branch 2.7.4.9 which doesn't have any change on this this module. I'm not sure how I could easily reproduce the use-case with a simple code at it seems to requires specific timing conditions to get a queued DNS request.

My analysis so far is that when a DNS request is queued, hostbyName() wait for wifi_dns_found_callback to update aResult. But this happens if and only if the DNS request is ok.

Arduino/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp

Lines 716 to 718 in 1b922ed

	if(ipaddr) {
	(*reinterpret_cast<IPAddress*>(callback_arg)) = IPAddress(ipaddr);
	}

On exit of the delay(timeout_ms), hostByName() test if the resolution was successful by testing aResult.isSet()

Arduino/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp

Lines 633 to 641 in 1b922ed

	} else if(err == ERR_INPROGRESS) {
	_dns_lookup_pending = true;
	delay(timeout_ms);
	// will resume on timeout or when wifi_dns_found_callback fires
	_dns_lookup_pending = false;
	// will return here when dns_found_callback fires
	if(aResult.isSet()) {
	err = ERR_OK;
	}

The problem is that the test result is always true because aResult is initialized at the beginning of the hostByName function to INADDR_NONE (255.255.255.255).

Arduino/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp

Line 617 in 1b922ed

aResult = static_cast<uint32_t>(INADDR_NONE);

My suggestion would be to remove the initialisation of aResult so that testing aResult.isSet() becomes valid but I am unsure why it was initialized at first (it used to initialized to INADDR_ANY (0) 1 year ago and that was changed to INADDR_NONE - so someone consider that it should be).
Alternatively, replacing the isSet() test with a test for INADDR_NONE could work too as I don't think a proper DNS resolution would return INADDR_NONE.

I you want, I can make a PR if one of the 2 suggested change gets your approval.

MCVE Sketch

This is the code snippet in Tasmota where this happens. We had to add the test against 255.255.255.255 to prevent broadcasting of syslog messages in case of erroneous address returned by hostByName().

        IPAddress temp_syslog_host_addr;
        int ok = WiFi.hostByName(SettingsText(SET_SYSLOG_HOST), temp_syslog_host_addr);  // If sleep enabled this might result in exception so try to do it once using hash
        if (!ok || (0xFFFFFFFF == (uint32_t)temp_syslog_host_addr)) { // 255.255.255.255 is assumed a DNS problem
          TasmotaGlobal.syslog_level = 0;
          TasmotaGlobal.syslog_timer = SYSLOG_TIMER;
          AddLog(LOG_LEVEL_INFO, PSTR(D_LOG_APPLICATION "Loghost DNS resolve failed (%s). " D_RETRY_IN " %d " D_UNIT_SECOND), SettingsText(SET_SYSLOG_HOST), SYSLOG_TIMER);
          return;
        }

Debug Messages

[d-a-v]

but I am unsure why it was initialized at first (it used to initialized to INADDR_ANY (0) 1 year ago and that was changed to INADDR_NONE - so someone consider that it should be).

That was in #6865. I approved this change among others and it is right that INADDR_NONE wrongly triggers .isSet()==true.
We'd appreciate your PR proposal to initialize aResult to INADDR_ANY as it was before @barbudor. I don't think that was intended by @altelch.

I'm unusure whether .isSet() should be false with INADDR_NONE though.
When an IP address is set to this value, it is at least set with something.

Thanks for the investigation !

[barbudor]

@d-a-v
I have to apologies has I didn't dig enough last time. I should have looked into IPAddress::isSet() in the master branch and not only in the frozen framework used by Tasmota.
Here is what Tasmota has in the frozen 2.7.4.+9

bool IPAddress::isSet () const {
    return !ip_addr_isany(&_ip);
}

But current framework master code is

Arduino/cores/esp8266/IPAddress.cpp

Lines 34 to 36 in fdc295d

	bool IPAddress::isSet () const {
	return !ip_addr_isany(&_ip) && ((*this) != IPADDR_NONE);
	}

Changed 6 months ago in #7585

As neither IADDR_ANY nor IADDR_NONE would be valid DNS response, the whole code would work correctly with up to date core.
Sorry for this

Tagging @Jason2866 for info.