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

Fixed bug #73594 (dns_get_record() does not populate $additional out parameter) #2222

Closed
wants to merge 2 commits into
base: PHP-7.0
from

Conversation

5 participants
@weirdan
Contributor

weirdan commented Nov 25, 2016

No description provided.

@weirdan weirdan changed the title from Fixed bug #73594 to Fixed bug #73594 (dns_get_record() does not populate $additional out parameter) Nov 25, 2016

@php-pulls

This comment has been minimized.

Show comment
Hide comment
@php-pulls

php-pulls Nov 26, 2016

Comment on behalf of krakjoe at php.net:

labelling

php-pulls commented Nov 26, 2016

Comment on behalf of krakjoe at php.net:

labelling

@php-pulls php-pulls added the Bugfix label Nov 26, 2016

@weirdan

This comment has been minimized.

Show comment
Hide comment
@weirdan

weirdan Nov 27, 2016

Contributor

Travis check fail seems to be unrelated.

Contributor

weirdan commented Nov 27, 2016

Travis check fail seems to be unrelated.

@weirdan

This comment has been minimized.

Show comment
Hide comment
@weirdan

weirdan Dec 16, 2016

Contributor

Is there anything else I can do to get this merged? @weltling ?

Contributor

weirdan commented Dec 16, 2016

Is there anything else I can do to get this merged? @weltling ?

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Dec 16, 2016

Contributor

@weirdan oh, i didn't follow this bug. Thanks for the ping, will check soon.

Contributor

weltling commented Dec 16, 2016

@weirdan oh, i didn't follow this bug. Thanks for the ping, will check soon.

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Dec 18, 2016

Contributor

@weirdan the test doesn't pass with 5.6, but it was supposed to pass with it, right? Otherwise, there seems to be a mistake in the documentation, as the fifth argument is documented as pass-by-ref, which is not happening indeed. To the test, could you please give a hint?

Thanks.

Contributor

weltling commented Dec 18, 2016

@weirdan the test doesn't pass with 5.6, but it was supposed to pass with it, right? Otherwise, there seems to be a mistake in the documentation, as the fifth argument is documented as pass-by-ref, which is not happening indeed. To the test, could you please give a hint?

Thanks.

@weirdan

This comment has been minimized.

Show comment
Hide comment
@weirdan

weirdan Dec 18, 2016

Contributor

tl;dr: the test works with 5.6, but it's fragile and may fail in your specific environment. The documentation is right

The test, unfortunately, is very sensitive to environment, as it queries the default DNS recursor for your host (could be your provider's, or your home router, or something else) and there's no easy way to change that. DNS servers have their own ideas on what to put to that additional section, and yours may have decided to not put anything. Here's, for example, two dig results, one using my office VPN and another one is from my direct home connection:
Office:

;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 25071
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 13, ADDITIONAL: 9

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;php.net.			IN	MX

;; ANSWER SECTION:
php.net.		29	IN	MX	0 php-smtp2.php.net.

;; AUTHORITY SECTION:
net.			41501	IN	NS	l.gtld-servers.net.
net.			41501	IN	NS	b.gtld-servers.net.
net.			41501	IN	NS	g.gtld-servers.net.
net.			41501	IN	NS	j.gtld-servers.net.
net.			41501	IN	NS	h.gtld-servers.net.
net.			41501	IN	NS	f.gtld-servers.net.
net.			41501	IN	NS	e.gtld-servers.net.
net.			41501	IN	NS	d.gtld-servers.net.
net.			41501	IN	NS	i.gtld-servers.net.
net.			41501	IN	NS	c.gtld-servers.net.
net.			41501	IN	NS	a.gtld-servers.net.
net.			41501	IN	NS	m.gtld-servers.net.
net.			41501	IN	NS	k.gtld-servers.net.

;; ADDITIONAL SECTION:
c.gtld-servers.net.	67311	IN	A	192.26.92.30
d.gtld-servers.net.	51065	IN	A	192.31.80.30
e.gtld-servers.net.	26125	IN	A	192.12.94.30
f.gtld-servers.net.	17658	IN	A	192.35.51.30
h.gtld-servers.net.	3916	IN	A	192.54.112.30
i.gtld-servers.net.	16217	IN	A	192.43.172.30
j.gtld-servers.net.	39478	IN	A	192.48.79.30
m.gtld-servers.net.	56495	IN	A	192.55.83.30

;; Query time: 60 msec
;; SERVER: 192.168.3.2#53(192.168.3.2)
;; WHEN: Sun Dec 18 05:09:53 EET 2016
;; MSG SIZE  rcvd: 411

Home:

; <<>> DiG 9.10.3-P4-Debian <<>> -tmx php.net
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 17019
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 4, ADDITIONAL: 8

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;php.net.			IN	MX

;; ANSWER SECTION:
php.net.		30	IN	MX	0 php-smtp2.php.net.

;; AUTHORITY SECTION:
php.net.		157084	IN	NS	dns4.easydns.info.
php.net.		157084	IN	NS	dns1.easydns.com.
php.net.		157084	IN	NS	dns2.easydns.net.
php.net.		157084	IN	NS	dns3.easydns.org.

;; ADDITIONAL SECTION:
dns1.easydns.com.	165696	IN	A	162.159.24.53
dns1.easydns.com.	159557	IN	AAAA	2400:cb00:2049:1::a29f:1835
dns2.easydns.net.	279	IN	A	198.41.222.254
dns2.easydns.net.	172479	IN	AAAA	2400:cb00:2049:1::c629:defe
dns3.easydns.org.	60	IN	A	64.68.196.10
dns4.easydns.info.	16996	IN	A	194.0.2.19
dns4.easydns.info.	18544	IN	AAAA	2001:678:5::13

;; Query time: 13 msec
;; SERVER: 127.0.0.1#53(127.0.0.1)
;; WHEN: Sun Dec 18 05:13:36 EET 2016
;; MSG SIZE  rcvd: 328

Documentation is actually right, that parameter should be passed by ref, as it's a DNS server that may return something in additional section. As you can see from the code, that array is populated when processing the reply. If wasn't meant to be passed by ref, there would be no reason to populate it inside this function.

Contributor

weirdan commented Dec 18, 2016

tl;dr: the test works with 5.6, but it's fragile and may fail in your specific environment. The documentation is right

The test, unfortunately, is very sensitive to environment, as it queries the default DNS recursor for your host (could be your provider's, or your home router, or something else) and there's no easy way to change that. DNS servers have their own ideas on what to put to that additional section, and yours may have decided to not put anything. Here's, for example, two dig results, one using my office VPN and another one is from my direct home connection:
Office:

;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 25071
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 13, ADDITIONAL: 9

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;php.net.			IN	MX

;; ANSWER SECTION:
php.net.		29	IN	MX	0 php-smtp2.php.net.

;; AUTHORITY SECTION:
net.			41501	IN	NS	l.gtld-servers.net.
net.			41501	IN	NS	b.gtld-servers.net.
net.			41501	IN	NS	g.gtld-servers.net.
net.			41501	IN	NS	j.gtld-servers.net.
net.			41501	IN	NS	h.gtld-servers.net.
net.			41501	IN	NS	f.gtld-servers.net.
net.			41501	IN	NS	e.gtld-servers.net.
net.			41501	IN	NS	d.gtld-servers.net.
net.			41501	IN	NS	i.gtld-servers.net.
net.			41501	IN	NS	c.gtld-servers.net.
net.			41501	IN	NS	a.gtld-servers.net.
net.			41501	IN	NS	m.gtld-servers.net.
net.			41501	IN	NS	k.gtld-servers.net.

;; ADDITIONAL SECTION:
c.gtld-servers.net.	67311	IN	A	192.26.92.30
d.gtld-servers.net.	51065	IN	A	192.31.80.30
e.gtld-servers.net.	26125	IN	A	192.12.94.30
f.gtld-servers.net.	17658	IN	A	192.35.51.30
h.gtld-servers.net.	3916	IN	A	192.54.112.30
i.gtld-servers.net.	16217	IN	A	192.43.172.30
j.gtld-servers.net.	39478	IN	A	192.48.79.30
m.gtld-servers.net.	56495	IN	A	192.55.83.30

;; Query time: 60 msec
;; SERVER: 192.168.3.2#53(192.168.3.2)
;; WHEN: Sun Dec 18 05:09:53 EET 2016
;; MSG SIZE  rcvd: 411

Home:

; <<>> DiG 9.10.3-P4-Debian <<>> -tmx php.net
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 17019
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 4, ADDITIONAL: 8

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;php.net.			IN	MX

;; ANSWER SECTION:
php.net.		30	IN	MX	0 php-smtp2.php.net.

;; AUTHORITY SECTION:
php.net.		157084	IN	NS	dns4.easydns.info.
php.net.		157084	IN	NS	dns1.easydns.com.
php.net.		157084	IN	NS	dns2.easydns.net.
php.net.		157084	IN	NS	dns3.easydns.org.

;; ADDITIONAL SECTION:
dns1.easydns.com.	165696	IN	A	162.159.24.53
dns1.easydns.com.	159557	IN	AAAA	2400:cb00:2049:1::a29f:1835
dns2.easydns.net.	279	IN	A	198.41.222.254
dns2.easydns.net.	172479	IN	AAAA	2400:cb00:2049:1::c629:defe
dns3.easydns.org.	60	IN	A	64.68.196.10
dns4.easydns.info.	16996	IN	A	194.0.2.19
dns4.easydns.info.	18544	IN	AAAA	2001:678:5::13

;; Query time: 13 msec
;; SERVER: 127.0.0.1#53(127.0.0.1)
;; WHEN: Sun Dec 18 05:13:36 EET 2016
;; MSG SIZE  rcvd: 328

Documentation is actually right, that parameter should be passed by ref, as it's a DNS server that may return something in additional section. As you can see from the code, that array is populated when processing the reply. If wasn't meant to be passed by ref, there would be no reason to populate it inside this function.

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Dec 18, 2016

Contributor

The fifth argument is called $raw - it can't be set by ref currently, at least in 5.6, but it's documented to be so. That's what i was talking about.

You're right, the test passed on another machine, without any preparations. I see now in the ticket, that you set an additional nameserver in /etc/resolv.conf. This is not going to work for the arbitrary test environment, so it is should not be done. The test will be skip on Travis as an online test, though. IMO, having some code to skip on unsuitable environment would make sense. Maybe calling dig directly in skipf section, and if it doesn't deliver the required sections, the test should not run. If dig is not available on the system, it should skip as well, as otherwise a false positive could be delivered.

Another thing also - as the $authns arg is touched, it should be tested as well. Probably would make sense to add a separate test for it.

Thanks.
Thanks.

Contributor

weltling commented Dec 18, 2016

The fifth argument is called $raw - it can't be set by ref currently, at least in 5.6, but it's documented to be so. That's what i was talking about.

You're right, the test passed on another machine, without any preparations. I see now in the ticket, that you set an additional nameserver in /etc/resolv.conf. This is not going to work for the arbitrary test environment, so it is should not be done. The test will be skip on Travis as an online test, though. IMO, having some code to skip on unsuitable environment would make sense. Maybe calling dig directly in skipf section, and if it doesn't deliver the required sections, the test should not run. If dig is not available on the system, it should skip as well, as otherwise a false positive could be delivered.

Another thing also - as the $authns arg is touched, it should be tested as well. Probably would make sense to add a separate test for it.

Thanks.
Thanks.

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Dec 18, 2016

Member

@weltling You're right, the docs are wrong here. Fixed in http://svn.php.net/viewvc?view=revision&revision=341411. The raw argument is also missing from the proto comment in the code.

I think it's okay to merge this fix without a test. Right now get_dns_record() does not have any tests, presumably because of the problems discussed here. It certainly would be good to have tests for this function, but I don't think we should require them for this specific small fix.

Member

nikic commented Dec 18, 2016

@weltling You're right, the docs are wrong here. Fixed in http://svn.php.net/viewvc?view=revision&revision=341411. The raw argument is also missing from the proto comment in the code.

I think it's okay to merge this fix without a test. Right now get_dns_record() does not have any tests, presumably because of the problems discussed here. It certainly would be good to have tests for this function, but I don't think we should require them for this specific small fix.

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Dec 18, 2016

Contributor

@nikic, thanks for the doc fix. Yeah, the patch itself is trivial, i was only giving the comment to the test already supplied. @weirdan, if you're able to improve the test, please do, otherwise please remove the test.

Thanks.

Contributor

weltling commented Dec 18, 2016

@nikic, thanks for the doc fix. Yeah, the patch itself is trivial, i was only giving the comment to the test already supplied. @weirdan, if you're able to improve the test, please do, otherwise please remove the test.

Thanks.

Skip the tests when local resolver does not behave
* Added SKIPIF sections to check local resolver
* Added test to check $authns parameter
@KalleZ

This comment has been minimized.

Show comment
Hide comment
@KalleZ

KalleZ Dec 18, 2016

Contributor

This won't work on Windows, as the dig command is not available

Contributor

KalleZ commented on ext/standard/tests/network/bug73594.phpt in d9643ac Dec 18, 2016

This won't work on Windows, as the dig command is not available

This comment has been minimized.

Show comment
Hide comment
@weirdan

weirdan Dec 18, 2016

Contributor

The test would get skipped then, wouldn't it?

Contributor

weirdan replied Dec 18, 2016

The test would get skipped then, wouldn't it?

This comment has been minimized.

Show comment
Hide comment
@KalleZ

KalleZ Dec 18, 2016

Contributor

It should yes, but it seems really odd to skip Windows just for that when the function and its arguments is implemented on Windows.

After all, its a bug fix test

Contributor

KalleZ replied Dec 18, 2016

It should yes, but it seems really odd to skip Windows just for that when the function and its arguments is implemented on Windows.

After all, its a bug fix test

This comment has been minimized.

Show comment
Hide comment
@weirdan

weirdan Dec 18, 2016

Contributor

But is there a way (on windows) to ensure the resolver returns the sections we're looking for (authority and additional)?

Contributor

weirdan replied Dec 18, 2016

But is there a way (on windows) to ensure the resolver returns the sections we're looking for (authority and additional)?

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Dec 18, 2016

Contributor

I was trying this on Windows already, but seems even 5.6 that is supposed to work, never delivers. That on two different machines. It's likely to have different issues in DNS setup anyway, so a separate investigation would make sense.

Thanks.

Contributor

weltling replied Dec 18, 2016

I was trying this on Windows already, but seems even 5.6 that is supposed to work, never delivers. That on two different machines. It's likely to have different issues in DNS setup anyway, so a separate investigation would make sense.

Thanks.

This comment has been minimized.

Show comment
Hide comment
@weirdan

weirdan Dec 18, 2016

Contributor

So it sounds that removing the test was actually a good idea. Thoughts? @KalleZ , would you prefer a no test to a test that gets skipped when external tool (dig) is not available?

Contributor

weirdan replied Dec 18, 2016

So it sounds that removing the test was actually a good idea. Thoughts? @KalleZ , would you prefer a no test to a test that gets skipped when external tool (dig) is not available?

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Dec 18, 2016

Contributor

Well, it goes on other platform at least. I'm not a DNS guy, i guess it likely to require some non standard tools on Windows, not talking about the system setup. The function itself delivers, but i couldn't get it working with this particular case with extra sections, that's what i meant to say.

Thanks.

Contributor

weltling replied Dec 18, 2016

Well, it goes on other platform at least. I'm not a DNS guy, i guess it likely to require some non standard tools on Windows, not talking about the system setup. The function itself delivers, but i couldn't get it working with this particular case with extra sections, that's what i meant to say.

Thanks.

This comment has been minimized.

Show comment
Hide comment
@KalleZ

KalleZ Dec 18, 2016

Contributor

If @weltling thinks its fine to skip then I'm good too. As for dig on Windows it requires a third party install and I don't think we should rely on that, and I don't know if there is any other way with the defaults tools provided to obtain this information on CLI. Either way if there is no easy way to do it, I'm fine without :)

Contributor

KalleZ replied Dec 18, 2016

If @weltling thinks its fine to skip then I'm good too. As for dig on Windows it requires a third party install and I don't think we should rely on that, and I don't know if there is any other way with the defaults tools provided to obtain this information on CLI. Either way if there is no easy way to do it, I'm fine without :)

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Dec 18, 2016

Contributor

@KalleZ yep, unfortunately no easy way for this specific kind of test on Windows, Though, I've checked the Winows function variant, and args spec was broken there, too. So fixed that. Otherwise, seems the default DNS service doesn't deliver the extended information, required for the test, in first place. The tools i've tried like nslookup or powershell, don't as well. Maybe it's something about the local DNS service configuration, like on Linux previously, or something else, for now it should be fine.

Thanks.

Contributor

weltling replied Dec 18, 2016

@KalleZ yep, unfortunately no easy way for this specific kind of test on Windows, Though, I've checked the Winows function variant, and args spec was broken there, too. So fixed that. Otherwise, seems the default DNS service doesn't deliver the extended information, required for the test, in first place. The tools i've tried like nslookup or powershell, don't as well. Maybe it's something about the local DNS service configuration, like on Linux previously, or something else, for now it should be fine.

Thanks.

This comment has been minimized.

Show comment
Hide comment
@KalleZ

KalleZ Dec 18, 2016

Contributor

Alright, lets leave it as is then :)

Contributor

KalleZ replied Dec 18, 2016

Alright, lets leave it as is then :)

@weirdan

This comment has been minimized.

Show comment
Hide comment
@weirdan

weirdan Dec 18, 2016

Contributor

@weltling fixed tests as per your suggestions

Contributor

weirdan commented Dec 18, 2016

@weltling fixed tests as per your suggestions

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Dec 18, 2016

Contributor

Merged with c78fd45. Thanks @weirdan for putting the effort into the QA.

Contributor

weltling commented Dec 18, 2016

Merged with c78fd45. Thanks @weirdan for putting the effort into the QA.

@weltling weltling closed this Dec 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment