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

dnsdist: Prevent race between the DoH handling threads #9278

Merged
merged 2 commits into from Jul 2, 2020

Conversation

rgacogne
Copy link
Member

Short description

This PR fixes a race between the different threads involved in DoH handling, leading to use-after-free and a crash in some cases. This is done by making sure that the h2o req object is only accessed from the main DoH thread, where it can't be altered by h2o in the meantime, and by preventing any access to it once the request has been canceled by h2o. It also makes sure that the pointer to our internal DOHUnit object is cleared from the h2o request pool before releasing that DOHUnit object.
This change involves a penalty cost because we now need to copy the HTTP headers, if any, because we need to access them from a different thread than the main DoH one and the internal memory handled by h2o could be gone then, or even be altered under our feet.

This issue was discovered by Tomas Krizek from CZ.NIC while using the DNS Shotgun tool, and reported to us. Many thanks for that!

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

- clean up the pointer in pool memory when releasing a DOHUnit so that we
  don't try to access it later when the memory pool is destroyed ;
- clean up the 'self' pointer when the memory pool is destroyed so we
  don't try to access it when the DOHUnit is released.
@nicki-krizek
Copy link

I've tested this version and dnsdist now seems to be stable even under heavy DoH load.

Thanks for the prompt fix!

@rgacogne
Copy link
Member Author

rgacogne commented Jul 2, 2020

Thanks a lot for the initial report, your wonderful help to reproduce the issue and testing the fix, that's very much appreciated!

@rgacogne rgacogne merged commit 4fccd9e into PowerDNS:master Jul 2, 2020
@rgacogne rgacogne deleted the ddist-doh-self-cleanup-vect branch July 2, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants