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: Only set recursion protection once we know we do not return #10848

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

omoerbeek
Copy link
Member

@omoerbeek omoerbeek commented Oct 15, 2021

This is not a fix for the tsan errors we're seeing, but worthwhile to fix anyway.

Also catch exception thrown by executeCode. (I'm assuming it can throw an exception).

Note that the function is not thread-safe both due to the use of a global and opendir and readdir calls. Now as far as I understand the dnsdist startup seqeunce, this should not be a problem as dnssdist is still single threaded at the point of call. But if by accident includeDir is called from several threads simultaneously, things can go wrong.

Short description

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)
  • checked that this code was merged to master

…f an error.

Also catch exception thrown by executeCode.
Copy link
Member

@rgacogne rgacogne 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 not a fix for the tsan errors we're seeing, but worthwhile to fix anyway.

Indeed, thanks!

Also catch exception thrown by executeCode. (I'm assuming it can throw an exception).

It can, although dnsdist should exit if that happens so I'm not too worried about that global.

Note that the function is not thread-safe both due to the use of a global and opendir and readdir calls. Now as far as I understand the dnsdist startup seqeunce, this should not be a problem as dnssdist is still single threaded at the point of call. But if by accident includeDir is called from several threads simultaneously, things can go wrong.

Everything that involves the global Lua context will go wrong if called from several threads simultaneously, to be clear.

@omoerbeek omoerbeek merged commit f9f6117 into PowerDNS:master Oct 15, 2021
@omoerbeek omoerbeek deleted the dnsdist-includedir-recursion branch October 15, 2021 08:02
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