fix openbsd support in auth and dnsdist #3962

Merged
merged 7 commits into from Jun 28, 2016

Projects

None yet

4 participants

@Habbie
Member
Habbie commented Jun 8, 2016 edited

fixes #3009

@Habbie
Member
Habbie commented Jun 8, 2016

3rd commit basically reverts 9af87ad - have not given this deeper thought yet.

@Habbie
Member
Habbie commented Jun 8, 2016

@fobser this PR may be relevant to your interest

@Habbie
Member
Habbie commented Jun 8, 2016

Review welcome at this point. I have some minor nits of my own left that I will fix before merge.

@Habbie Habbie changed the title from [WIP] fix openbsd support in all our products to [WIP] fix openbsd support in auth and dnsdist Jun 8, 2016
@pieterlexis
Member

nit: maybe guard the includes as they are not required on non-OpenBSD?

@Habbie
Member
Habbie commented Jun 8, 2016

Not going to guard unistd.h; but yes, history.h is a candidate for guarding.

@Habbie
Member
Habbie commented Jun 8, 2016 edited

OpenBSD CURRENT (snapshot 6 June 2016) does not need the sendmsg ('anti-valgrind') change, by the way.

@rgacogne rgacogne commented on the diff Jun 8, 2016
pdns/misc.cc
@@ -845,7 +845,6 @@ void addCMsgSrcAddr(struct msghdr* msgh, void* cmsgbuf, const ComboAddress* sour
memset(pkt, 0, sizeof(*pkt));
pkt->ipi6_addr = source->sin6.sin6_addr;
pkt->ipi6_ifindex = itfIndex;
- msgh->msg_controllen = cmsg->cmsg_len; // makes valgrind happy and is slightly better style
@rgacogne
rgacogne Jun 8, 2016 Member

The initial commit introducing this line, 9af87ad, points to an interesting discussion on the matter: https://codereview.chromium.org/3026044
rfc3542 states, in section 20.2 "The cmsghdr Structure": "While sending an application may or may not include padding at the end of last ancillary data in msg_controllen and implementations must accept both as valid.", and then uses CMSG_SPACE() in 21.1.

@Habbie
Habbie Jun 8, 2016 Member

Without having read it all, are you saying that OpenBSD was wrong to complain?

@rgacogne
rgacogne Jun 8, 2016 Member

Well, yes and I wonder why, but valgrind is wrong too. After thinking about it a little more, this patch fixes a known issue and complies with the man page, so it's the right thing to do.

@Habbie
Habbie Jun 9, 2016 Member

Sounds good. I'll stick a comment in for when somebody finds it with valgrind again.

@Habbie
Member
Habbie commented Jun 8, 2016

Updated my OpenBSD current comment because I was wrong. 5.9 and current are completely identical in this behaviour.

@Habbie Habbie changed the title from [WIP] fix openbsd support in auth and dnsdist to fix openbsd support in auth and dnsdist Jun 9, 2016
@Habbie
Member
Habbie commented Jun 9, 2016

Ok, I feel this is done. Ready for merge and review.

@fobser
Contributor
fobser commented Jun 9, 2016

Thanks for doing this. I tried to compile 4.0.0-beta1 the other day on OpenBSD and it was failing. Didn't have the time to look into it then and report on it. With these patches the port compiles. Haven't tried running it yet though.

@Habbie
Member
Habbie commented Jun 9, 2016

@fobser I did a few of our regression suites with it and they fully pass except the few tests that try to pass -p to dig ;)

@rgacogne rgacogne commented on the diff Jun 10, 2016
pdns/nameserver.cc
if(p->d_anyLocal) {
addCMsgSrcAddr(&msgh, cbuf, p->d_anyLocal.get_ptr(), 0);
}
- else {
- msgh.msg_control=NULL;
- }
DLOG(L<<Logger::Notice<<"Sending a packet to "<< p->getRemote() <<" ("<< buffer.length()<<" octets)"<<endl);
@rgacogne
rgacogne Jun 10, 2016 Member

I know this PR is about auth and dnsdist, but it looks like we have the exact same construct in two places in pdns_recursor.cc, perhaps we could apply the same fix there?

@Habbie
Habbie Jun 10, 2016 Member

Oh, that's a good idea. I did not look into recursor when I realised it would never run for lack of both ucontext and boost context.

@Habbie
Habbie Jun 10, 2016 Member

added!

@pieterlexis
Member

Is this ready to be merged?

@Habbie
Member
Habbie commented Jun 28, 2016

Yes

@pieterlexis pieterlexis merged commit 6590f05 into PowerDNS:master Jun 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Habbie Habbie deleted the Habbie:openbsd branch Jun 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment