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

Fix unnecessary DNS query in hostByName and deadlock in ssl_client #7351

Merged
merged 18 commits into from Dec 7, 2022
Merged

Fix unnecessary DNS query in hostByName and deadlock in ssl_client #7351

merged 18 commits into from Dec 7, 2022

Conversation

cziter15
Copy link
Contributor

@cziter15 cziter15 commented Oct 14, 2022

Description of Change

Pull request to fix problems found when auditing WiFiClientSecure implementation used by ksIotFrameworkLib.

I have few IoT devices running 24/7 with MQTT. Recently added SSL support and identified that unit based on ESP32 is unable to reconnect after losing connection. I've been looking at WiFi code handling in my library, found some bugs, fixing them improved situation a little bit, but still sometimes device was unable to get back online.

After hours of testing by trial-and-error remotely, I have decided to get pcb on my desk and quickly identified few bugs related to WiFiClientSecure.

This PR aims to fix these bugs:

  • Fixes possibility to stuck inside while loop in function send_ssl_data. When retry is triggered and then internet connection dies, buffer can be overloaded with data causing execution to block there. Adding timeout prevents from stucking forever.
  • Removes unnecessary DNS query flow, when IP address is passed as a hostname inside hostByName function. Generally speaking there is a strange flow of coverting IPAddress (when using IPAddress variant of connect method) to a string, then passing it to hostByName function in WiFiClientSecure to... get IPAddress back, involving whole mechanism of waiting for DNS response.

Tests scenarios

Tested on custom board with ESP-WROOM-32 module without PSRAM. Latest master with applied changes. Looks like ESP32 device is getting back online like other ESP8266 based units.

Related links

Closes #7350 #7356

@cziter15 cziter15 changed the title Fix hostByName to avoid asking DNS when valid IP is passed via hostna… Fix unnecessary DNS query in hostByName, fix timeout in ssl_client Oct 15, 2022
@cziter15
Copy link
Contributor Author

cziter15 commented Oct 15, 2022

Anyway, it looks like handshake_timeout might not be required to be explicitly defined. Maybe handling it all under socket timeout is a good way?

@VojtechBartoska VojtechBartoska added the Area: BT&Wifi BT & Wifi related issues label Oct 21, 2022
@cziter15 cziter15 changed the title Fix unnecessary DNS query in hostByName, fix timeout in ssl_client Fix unnecessary DNS query in hostByName and deadlock in ssl_client Oct 29, 2022
@VojtechBartoska VojtechBartoska added this to the 2.0.6 milestone Nov 16, 2022
Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

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

Nice PR, @cziter15 ! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BT&Wifi BT & Wifi related issues
Projects
Development

Successfully merging this pull request may close these issues.

DNS query is issued in ssl_client's start_ssl_client method, even when IP address is provided
4 participants