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 race condition in WiFiGenericClass::hostByName #8672

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

MattiasTF
Copy link
Contributor

Description of Change

dns_gethostbyname, as used in hostByName, is required to run in lwIP’s TCP/IP context. This can be verified by enabling LWIP_CHECK_THREAD_SAFETY in the sdkconfig.

Calling dns_gethostbyname from the Arduino task can trigger race conditions in lwIP or lower layers. One possibility is a corruption of IDF's Ethernet buffers, causing an unstoppable flood of "insufficient TX buffer size" errors, effectively severing all Ethernet connectivity.

hostByName cannot be executed in the TCP/IP context because it blocks until completion, preventing the TCP/IP task from receiving the DNS response to unblock itself, causing a deadlock. Therefore, this PR patches hostByName to execute dns_gethostbyname in the TCP/IP context while still blocking only the Arduino task.

Tests scenarios

I have tested my PR on Arduino-esp32 master as of 02e31b4 on a custom ESP32 board.

@CLAassistant
Copy link

CLAassistant commented Sep 26, 2023

CLA assistant check
All committers have signed the CLA.

@VojtechBartoska VojtechBartoska added Area: WiFi Issue related to WiFi Status: Review needed Issue or PR is awaiting review labels Sep 27, 2023
@VojtechBartoska VojtechBartoska added this to the 3.0.0-RC1 milestone Nov 28, 2023
@me-no-dev
Copy link
Member

This is good to have! @MattiasTF could you please fix the merge conflicts, so that we can merge this?

@MattiasTF
Copy link
Contributor Author

MattiasTF commented Dec 4, 2023

What would be your preferred fix? I can rebase the branch to give you a single patch.

@me-no-dev
Copy link
Member

@MattiasTF that would be great. We just can't merge it right now and can not rebase it for you, because you did not select to allow us to do that when you created the PR.

dns_gethostbyname, as used in hostByName, is required to run in lwIP's TCP/IP
context. This can be verified by enabling LWIP_CHECK_THREAD_SAFETY in the
sdkconfig.

Calling dns_gethostbyname from the Arduino task can trigger race conditions
in lwIP or lower layers. One possibility is a corruption of IDF's Ethernet
buffers, causing an unstoppable flood of "insufficient TX buffer size" errors,
effectively severing all Ethernet connectivity.

This patch makes sure to call dns_gethostbyname from lwIP's TCP/IP context.
@MattiasTF
Copy link
Contributor Author

MattiasTF commented Dec 5, 2023

There we go.

We just can't merge it right now and can not rebase it for you, because you did not select to allow us to do that when you created the PR.

Thanks for the info. Can’t seem to find that option at the moment when creating a new PR, though.

@me-no-dev
Copy link
Member

The checkbox next to the green "Create Pull Request" button

MicrosoftTeams-image (1)

@MattiasTF
Copy link
Contributor Author

Thanks, but I just found out that that option only exists when creating PRs from user-owned forks. It’s not possible to use that option with forks living in company repos, which is why I don’t have it.

@me-no-dev
Copy link
Member

That is good to know! Thanks for researching it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: WiFi Issue related to WiFi Status: Review needed Issue or PR is awaiting review
Projects
Development

Successfully merging this pull request may close these issues.

4 participants