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

socksproxy: Fix localhost DNS and use-after-free crash #10332

Merged
merged 8 commits into from
Mar 11, 2025

Conversation

oskirby
Copy link
Collaborator

@oskirby oskirby commented Mar 11, 2025

Description

In continuing my testing with the socksproxy and the DNS resolution woes, I came across a couple of bugs that can lead to breakage.

The first issue is that when selecting DNS servers, when the first choice is a link-local IPv6 address, we don't set a scope ID in the address, which can prevent c-ares from being able to connect to it. We can fix this just by calling QHostAddress::setScopeId() when building the list of DNS servers. While implementing this, I also performed a bit of cleanup by moving the DNS server selection into WindowsBypass::updateNamserver()

The second issue, which was a bit trickier to track down, is that we have a use-after-free bug in the socks proxy that can occur between the c-ares resolver thread and the Qt thread. This happens when the Socks5Connection is closed before DNS resolution finishes, in which case the arg provided to the c-ares callback points to freed memory. To resolve this, we need to verify that the QObject is valid before invoking the onResolutionFinished() method. We add a QHash map to store the objects that requested resolution, and use the QObject::destroyed() signal to track when they are garabge collected.

Reference

Based upon PR #10317 by @strseb
JIRA Issues:

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

@oskirby oskirby force-pushed the naomi-proxy-dns-fixes-2 branch from e1a0253 to cf92b44 Compare March 11, 2025 02:30
@oskirby oskirby changed the title Naomi proxy dns fixes 2 socksproxy: Fix localhost DNS and use-after-free crash Mar 11, 2025
@oskirby oskirby force-pushed the naomi-proxy-dns-fixes-2 branch from cf92b44 to 414ef70 Compare March 11, 2025 03:57
@oskirby oskirby marked this pull request as ready for review March 11, 2025 05:00
@oskirby oskirby requested review from strseb and mcleinman March 11, 2025 05:00
Copy link
Collaborator

@mcleinman mcleinman left a comment

Choose a reason for hiding this comment

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

This is far from my area of expertise, but from what I can see it looks good. Thanks!

@oskirby oskirby merged commit d57e84b into main Mar 11, 2025
120 checks passed
@oskirby oskirby deleted the naomi-proxy-dns-fixes-2 branch March 11, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants