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: Wrap GnuTLS and OpenSSL pointers in smart pointers #7064

Merged
merged 1 commit into from Oct 15, 2018

Conversation

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Oct 12, 2018

Short description

This PR is based on #7060 but wraps the naked pointers returned by GnuTLS and OpenSSL in smart pointers instead of carefully tracking them.
It needs a more serious review, though.

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)
@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Oct 12, 2018
@rgacogne rgacogne requested a review from chbruyand Oct 12, 2018
@rgacogne rgacogne force-pushed the rgacogne:dnsdist-tls-accept-leak-smart branch 2 times, most recently from a7440ca to 8039c50 Oct 12, 2018
@zeha
Copy link
Collaborator

@zeha zeha commented Oct 13, 2018

OpenSSL docs have this so say:

Normally the session cache is checked for expired sessions every 255 connections using the SSL_CTX_flush_sessions function. Since this may lead to a delay which cannot be controlled, the automatic flushing may be disabled and SSL_CTX_flush_sessions can be called explicitly by the application.

I don't know if this is a significant delay or if this should be considered for dnsdist.

@rgacogne
Copy link
Member Author

@rgacogne rgacogne commented Oct 15, 2018

I don't know if this is a significant delay or if this should be considered for dnsdist.

This might be, but I'm not sure how we could do better by managing that ourselves? Calling SSL_CTX_flush_sessions() will block anyone from accessing the session store anyway, even if we called it from a different thread, so the only thing we could do is to call it less often.
IMHO we can ignore it for now, until it happens to be an issue.

@zeha
Copy link
Collaborator

@zeha zeha commented Oct 15, 2018

This might be, but I'm not sure how we could do better by managing that ourselves? Calling SSL_CTX_flush_sessions() will block anyone from accessing the session store anyway, even if we called it from a different thread, so the only thing we could do is to call it less often.

Right. I was thinking calling it from the maintenance thread would at least give consistent behaviour. But I agree it's probably a good idea to delay this for now!

@rgacogne rgacogne force-pushed the rgacogne:dnsdist-tls-accept-leak-smart branch from 8039c50 to 8dd7033 Oct 15, 2018
Copy link
Member

@chbruyand chbruyand left a comment

LGTM

@rgacogne rgacogne merged commit 5bddfdd into PowerDNS:master Oct 15, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgacogne rgacogne deleted the rgacogne:dnsdist-tls-accept-leak-smart branch Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.