Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Fix #78479: Use gethostbyname2_r to specify AF_INET #6302
Conversation
|
Looks good to me, I wonder what happened in the test suite. |
|
On 8 October 2020 22:23:08 BST, Nicolas Thumann ***@***.***> wrote:
As @derickr latest commit (14d231b) on
`PHP-7.3` didn´t made it through the CI-Pipeline and I´m requesting to
merge into that branch, mine failed as well :/>
>
-- >
You are receiving this because you were mentioned.>
Reply to this email directly or view it on GitHub:>
#6302 (comment)
Can you link me to the exact spot of the test failure?
|
|
On 8 October 2020 22:41:04 BST, Nicolas Thumann ***@***.***> wrote:
> Can you link me to the exact spot of the test failure?>
>
Sure, e.g.
[Travis](https://travis-ci.org/github/php/php-src/jobs/733989398#L3283)
of
14d231b
That looks like a stupid test testing with data that can change (and did). I'll have a look tomorrow.
|
| @@ -1287,7 +1287,7 @@ struct hostent * gethostname_re (const char *host,struct hostent *hostbuf,char * | |||
| } | |||
|
|
|||
| while (( res = | |||
| gethostbyname_r(host,hostbuf,*tmphstbuf,*hstbuflen,&hp,&herr)) | |||
| gethostbyname2_r(host,AF_INET,hostbuf,*tmphstbuf,*hstbuflen,&hp,&herr)) | |||
nikic
Oct 9, 2020
Member
Can this be fixed without using gethostbyname2_r? This file already tests many different gethostbyname prototypes for cross-platform support, and we shouldn't add more unless unavoidable.
The current change is incorrect as-is, because it assumes that gethostbyname2_r is available whenever a six-argument prototype of gethostbyname_r is available.
Can this be fixed without using gethostbyname2_r? This file already tests many different gethostbyname prototypes for cross-platform support, and we shouldn't add more unless unavoidable.
The current change is incorrect as-is, because it assumes that gethostbyname2_r is available whenever a six-argument prototype of gethostbyname_r is available.
sundbry
Oct 9, 2020
I don't think it can be properly fixed without it. As in the original bug report, it was possible for a domain with a valid A record yet no AAAA record return a result for AF_INET6. In this system call we need to explicitly request an AF_INET result only.
Any result from gethostbyname_r with an INET6 return struct is useless in this context, it doesn't indicate that there is no record for a normal ipv4 address.
I don't think it can be properly fixed without it. As in the original bug report, it was possible for a domain with a valid A record yet no AAAA record return a result for AF_INET6. In this system call we need to explicitly request an AF_INET result only.
Any result from gethostbyname_r with an INET6 return struct is useless in this context, it doesn't indicate that there is no record for a normal ipv4 address.
n-thumann
Oct 9, 2020
•
Author
I think @nikic is right, also the issue would remain on other platforms as Linux is the only one to support gethostbyname2_r for specifying a type.
I think we can use getaddrinfo instead of gethostbyname_r or gethostbyname2_r (both marked as obsolescent). That works basically the same, but returns another struct addrinfo instead of struct hostent (shouldn´t be an issue, can be manually assigned) and as far as I could tell, the syntax/number of arguments is always the same (checked Linux, Solaris & HP-UX), so we should be able to remove the platform check, don´t we?
I think @nikic is right, also the issue would remain on other platforms as Linux is the only one to support gethostbyname2_r for specifying a type.
I think we can use getaddrinfo instead of gethostbyname_r or gethostbyname2_r (both marked as obsolescent). That works basically the same, but returns another struct addrinfo instead of struct hostent (shouldn´t be an issue, can be manually assigned) and as far as I could tell, the syntax/number of arguments is always the same (checked Linux, Solaris & HP-UX), so we should be able to remove the platform check, don´t we?
n-thumann
force-pushed the
n-thumann:bugfix/78479_gethostbyname
branch
from
db020d8
to
6d18256
|
Changed |
n-thumann
force-pushed the
n-thumann:bugfix/78479_gethostbyname
branch
2 times, most recently
from
73a2843
to
c916fa9
gethostbyname_r can return both AF_INET and AF_INET6 addresses, but php_gethostbyname will always memcopy the result into an in_addr (v4 only) struct.
n-thumann
force-pushed the
n-thumann:bugfix/78479_gethostbyname
branch
from
d02f9a4
to
7b7e81b
| nresults++; | ||
|
|
||
| hp = emalloc(sizeof(struct hostent)); | ||
| hp->h_name = strdup(host); |
sundbry
Oct 11, 2020
•
Suggested change
hp->h_name = strdup(host);
hp->h_name = strndup(host, _POSIX_HOST_NAME_MAX);
I suggest we guard against a buffer overflow attack here.
Would also need to #include <limits.h> to get _POSIX_HOST_NAME_MAX (should be 256 bytes)
| hp->h_name = strdup(host); | |
| hp->h_name = strndup(host, _POSIX_HOST_NAME_MAX); |
I suggest we guard against a buffer overflow attack here.
Would also need to #include <limits.h> to get _POSIX_HOST_NAME_MAX (should be 256 bytes)
|
I agree that switching to getaddrinfo is generally the right approach here. If you want this to actually land in PHP-7.3 rather than master, we can't change the behavior of php_network_gethostbyname() in this way, as it may break 3rd-party consumers. I'm also not sure it really makes sense to polyfill gethostbyname in this manner, it might make more sense to just switch uses of this function to call getaddrinfo directly (there's already quite a few usages). |
| #ifdef HAVE_GETHOSTBYNAME_R | ||
| #ifdef HAVE_GETADDRINFO | ||
| #ifdef PHP_WIN32 | ||
| #include "win32/ws2tcpip.h" #TODO: does this work? |
nikic
Oct 12, 2020
Member
It should probably be just #include <Ws2tcpip.h>. (We already have a couple usages of this header.)
It should probably be just #include <Ws2tcpip.h>. (We already have a couple usages of this header.)
|
So, to put it into a more actionable form... For PHP 7.3, I would replace the gethostbyname/gethostbynamel function with implementations based on getaddrinfo and leave it at that. For master, we can replace misc other usages and drop php_network_gethostbyname entirely. Only thing I'm unsure about is whether we need to guard by HAVE_GETADDRINFO. While it's used in some places, sockets.c notably uses getaddrinfo() unconditionally. That makes me think that we should just use it unconditionally, and let someone complain if it breaks their build :) |
gethostbyname_rcan return bothAF_INETandAF_INET6addresses, butphp_gethostbynamewill always memcopy the result into anin_addr(v4 only) struct. This PR therefore replacesgethostbyname_rwithgethostbyname2_rand sets the address type to be returned toAF_INET.Bug can be reproduced by enabling
inet6inresolv.conf, e.g. with Docker:docker run -it --rm --dns-opt inet6 php sh -c "echo '<?php echo gethostbyname(\"www.google.com\"); echo \"\\n\"; echo gethostbyname(\"downloads.wordpress.org\"); echo \"\\n\"; ?>' | php"will returnSee Bug #78479 for more information.