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

curl dns_entry related improvements #14195

Closed
wants to merge 3 commits into from
Closed

Conversation

icing
Copy link
Contributor

@icing icing commented Jul 16, 2024

Replace Curl_resolv_unlock() with Curl_resolv_unlink():

  • replace inuse member with refcount in Curl_dns_entry
  • pass Curl_dns_entry ** to unlink, so it gets always cleared
  • solve potential (but unlikley) UAF in FTP's handling of looked up Curl_dns_entry. Esp. do not use addr information after unlinking an entry.
    In reality, the unlink will not free memory, as the dns entry is still referenced by the hostcache. But this is not safe and relying on no other code pruning the cache in the meantime.
  • pass permanent flag when adding a dns entry instead of fixing timestamp afterwards.

url.c, fold several static *resolve_* functions into one.

@github-actions github-actions bot added the tests label Jul 16, 2024
lib/hostip.c Outdated Show resolved Hide resolved
- replace `inuse` member with `refcount` in Curl_dns_entry
- pass `Curl_dns_entry **` to unlink, so it gets always cleared
- solve potential (but unlikley) UAF in FTP's handling of looked
  up Curl_dns_entry. Esp. do not use addr information after
  unlinking an entry.
  In reality, the unlink will not free memory, as the dns entry
  is still referenced by the hostcache. But this is not safe and
  relying on no other code pruning the cache in the meantime.
@bagder
Copy link
Member

bagder commented Jul 20, 2024

You think this should merge before 8.9.0?

@icing
Copy link
Contributor Author

icing commented Jul 22, 2024

You think this should merge before 8.9.0?

I think it can wait.

@icing icing added the feature-window A merge of this requires an open feature window label Jul 22, 2024
@bagder bagder closed this in 5a9262a Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-window A merge of this requires an open feature window tests
Development

Successfully merging this pull request may close these issues.

2 participants