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

random engine #9016

Merged
merged 3 commits into from Apr 24, 2020
Merged

random engine #9016

merged 3 commits into from Apr 24, 2020

Conversation

omoerbeek
Copy link
Member

@omoerbeek omoerbeek commented Apr 8, 2020

Short description

Continuation of #9004, which does not allow me to push anymore.

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)

@omoerbeek omoerbeek mentioned this pull request Apr 8, 2020
3 tasks
@lgtm-com
Copy link

lgtm-com bot commented Apr 8, 2020

This pull request introduces 1 alert when merging c1fc00a into c1fdf91 - view on LGTM.com

new alerts:

  • 1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

}
return *this;
}
DNSName(const DNSName& a) = default;
Copy link
Member

@rgacogne rgacogne Apr 9, 2020

Choose a reason for hiding this comment

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

I know this is a work-in-progress but be careful that declaring a copy assignment operator or a copy constructor will prevent the move operations from being generated.

Copy link
Member Author

@omoerbeek omoerbeek Apr 9, 2020

Choose a reason for hiding this comment

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

Yes. But so far this is the only "solution" I could think of.

Copy link
Member

@rgacogne rgacogne left a comment

The general concept of this PR looks good to me. A few nits, and also it seems to miss quite a few other users of std::random_shuffle():

  • pdns/calidns.cc
  • pdns/dnsdist-lua-actions.cc
  • pdns/ws-auth.cc
  • pdns/signingpipe.cc
  • pdns/dynhandler.cc
  • pdns/dnsbulktest.cc
  • pdns/slavecommunicator.cc

return *this;
}
DNSName(const DNSName& a) = default;
DNSName(DNSName&& a) = default;
Copy link
Member

@rgacogne rgacogne Apr 20, 2020

Choose a reason for hiding this comment

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

Suggested change
DNSName(DNSName&& a) = default;
DNSName(DNSName&& a) = default;
DNSName& operator=(DNSName&& rhs)
{
if (this != &rhs) {
d_storage = std::move(rhs.d_storage);
}
return *this;
}

#include "dnsparser.hh"

// shuffle, maintaining some semblance of order
void pdns::shuffle(std::vector<DNSZoneRecord>& rrs)
Copy link
Member

@rgacogne rgacogne Apr 20, 2020

Choose a reason for hiding this comment

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

So if I understand correctly, this function assumes that we have first CNAMEs in the ANSWER section, then other ANSWER records? In which case it will then shuffle the second ones only. And Then then the same for additional records? I know it's has not been modified by this PR but I think some comment might help for the next time, especially since the move to a new file means the history will be harder to find.

pdns/shuffle.cc Outdated


// shuffle, maintaining some semblance of order
void pdns::shuffle(std::vector<DNSRecord>& rrs)
Copy link
Member

@rgacogne rgacogne Apr 20, 2020

Choose a reason for hiding this comment

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

And this one is mostly the same, except it also assumes that RRSIGs will come at the end of each section. It looks like it's never called directly, always from orderAndShuffle(), perhaps we could make it static?

@@ -553,8 +554,9 @@ DNSAction::Action SpoofAction::operator()(DNSQuestion* dq, std::string* ruleresu
}
}

static std::default_random_engine r;
Copy link
Member

@rgacogne rgacogne Apr 21, 2020

Choose a reason for hiding this comment

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

Unless std::default_random_engine::operator() is thread-safe, I don't think we should make r static.

Copy link
Member Author

@omoerbeek omoerbeek Apr 21, 2020

Choose a reason for hiding this comment

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

Indeed. I do not think we want to re-create the engine object all the time, since it might use a static seed (and it does afaks in at least the clang c++ lib). Maybe a thread local?

Copy link
Member

@rgacogne rgacogne Apr 21, 2020

Choose a reason for hiding this comment

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

How about placing it in the SpoofAction class? I can write that change if you would like me to :)

Copy link
Member Author

@omoerbeek omoerbeek Apr 21, 2020

Choose a reason for hiding this comment

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

Right. If that takes care of the multi-threaded access that is preferred. Please do so. There might be caveats I do not know with the DNSAction classes :-)

Copy link
Member

@rgacogne rgacogne Apr 21, 2020

Choose a reason for hiding this comment

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

On it!

Copy link
Member

@rgacogne rgacogne Apr 21, 2020

Choose a reason for hiding this comment

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

For a reason unknown to me Github doesn't allow me to open a pull request against your branch, so please pull 387221d from https://github.com/rgacogne/pdns/tree/random-engine :)

@omoerbeek
Copy link
Member Author

omoerbeek commented Apr 22, 2020

I think it is good to merge now. I'll squash it first though.

krionbsd and others added 3 commits Apr 22, 2020
std::random_shuffle is deprecated and removed starting from C++17, so we
might better use std::shuffle which uses URBG:
https://en.cppreference.com/w/cpp/named_req/UniformRandomBitGenerator
under the hood for a better random generation.
…tring

do not like that and throw an assertion.
@omoerbeek omoerbeek merged commit d4870ef into PowerDNS:master Apr 24, 2020
29 checks passed
@omoerbeek omoerbeek deleted the random-engine branch Apr 24, 2020
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

3 participants