You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2007/10/14 01:41:41 UTC
testipsub bogosty centered on getaddrinfo
Trying do decide who's at fault here, I tend to blame the test since it failed
to follow the style guidelines, and apr_ipsubnet_test since it's not named
according to apr conventions, so these are just two examples that this code
isn't well thought out. But let me break this down and ask which fix(es) are
appropriate, we'll start at the segfaulting test itself...
static void test_interesting_subnets(abts_case *tc, void *data)
{
struct {
const char *ipstr, *mask;
int family;
char *in_subnet, *not_in_subnet;
} testcases[] =
{
{"9.67", NULL, APR_INET, "9.67.113.15", "10.1.2.3"}
[...]
#if APR_HAVE_IPV6
[...]
,{"3FFE:8160::", "28", APR_INET6, "3ffE:816e:abcd:1234::1", "3ffe:8170::1"}
[All of the tests above work on win32, with ipv6 compiled but without ipv6 configured.
The problems lurk in the final two tests... note that these IP's aren't even legal
IPv6 addresses...]
,{"127.0.0.1", NULL, APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
,{"127.0.0.1", "8", APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
#endif /* IPV6 */
};
apr_ipsubnet_t *ipsub;
apr_sockaddr_t *sa;
apr_status_t rv;
int i, rc;
for (i = 0; i < sizeof testcases / sizeof testcases[0]; i++) {
rv = apr_ipsubnet_create(&ipsub, testcases[i].ipstr, testcases[i].mask, p);
ABTS_TRUE(tc, rv == APR_SUCCESS);
rv = apr_sockaddr_info_get(&sa, testcases[i].in_subnet, testcases[i].family, 0, 0, p);
[In the problem cases, apr_sockaddr_info_get returns SUCCESS, sa == NULL and
it's obviously related to how getaddrinfo works on win32, but as we'll examine
in a moment - any platform might just return an sa of NULL with or without an
error code. SO, ANY platform could explode...]
ABTS_TRUE(tc, rv == APR_SUCCESS);
rc = apr_ipsubnet_test(ipsub, sa);
This crashes, since sa-> is immediately dereferenced, in apr_ipsubnet_test,
and would continue to crash on any number of platforms if they encounter problems
with the test examples. Crashing is a totally unacceptable reaction and prevents
us from seeing what other issues might exist (and be interrelated).
First question of the test, should we dodge the apr_ipsubnet_test above if we
see a non-success rv? or if we see a null sa? or never dodge it, but accept NULL
to the apr_ipsubnet_test and return nomatch?
Now my question below is; which patches should be applied? I'll break them down
individually...
Index: network_io/unix/sockaddr.c
===================================================================
--- network_io/unix/sockaddr.c (revision 584444)
+++ network_io/unix/sockaddr.c (working copy)
@@ -346,12 +346,18 @@
}
error = getaddrinfo(hostname, servname, &hints, &ai_list);
#ifdef HAVE_GAI_ADDRCONFIG
- if (error == EAI_BADFLAGS && family == APR_UNSPEC) {
+ if ((!error && !ai_list)
+ || (error == EAI_BADFLAGS && family == APR_UNSPEC)) {
+#else
+ if (!error && !ai_list) {
+#endif
/* Retry with no flags if AI_ADDRCONFIG was rejected. */
hints.ai_flags = 0;
error = getaddrinfo(hostname, servname, &hints, &ai_list);
[We anticipated that flags could cause a problem and we fall back in the
case that we were told our flags are bad, but do we also want to fall back
and try without flags, if we encounter zero results for ai_list?]
+ if (!error && !ai_list) {
+ return EAI_NONAME + APR_OS_START_EAIERR;
+ }
}
-#endif
[The question comes down to, must we respond with an error if we have zero
ai_list members, or is APR_SUCCESS plus zero ai_list elements an anticipated
situation that the caller is expected to address?]
@@ -944,10 +947,10 @@
APR_DECLARE(int) apr_ipsubnet_test(apr_ipsubnet_t *ipsub, apr_sockaddr_t *sa)
{
+ if (!sa)
+ return 0; /* no match (idiots) */
+
#if APR_HAVE_IPV6
- /* XXX This line will segv on Win32 build with APR_HAVE_IPV6,
- * but without the IPV6 drivers installed.
- */
if (sa->sa.sin.sin_family == AF_INET) {
[Do we need to surpress idiocy such as the example use case, by guarding against
such NULL sa's here in the apr_ipsubnet_test?]
Comments please, from those of you more familiar with getaddrinfo than I am?
TIA,
Bill
Re: Re-asking for feedback: testipsub bogosty centered on getaddrinfo
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Still looking for feedback of the appropriate error code if we can't figure
out what the OS means by returning nothing with no error code from getaddrinfo.
Guess I'll start a daily ping on the issue, but I have no intention of rolling
or approving a release till we close this particular OS bug.
Bill
William A. Rowe, Jr. wrote:
> Colm was correct; the addresses that an ipv6-kernel-sans-ipv6-driver can't
> swallow on win32 are the IPv4-dotted-IPv6-prefixed formats. But it doesn't
> fail gracefully, it just returns success and no elts in the list, which
> will
> cause us to return NULL for the resulting sa, and crash soon afterwards.
>
> Do we
>
> * anticipate NULL sa's? (Seems silly)
> * emit an error? Which one is appropriate for "i just don't understand,
> but it doesn't mean your args were wrong"? Preferably something from
> the EAI family that MS *should* have returned.
>
> Bill
>
> William A. Rowe, Jr. wrote:
>> Trying do decide who's at fault here, I tend to blame the test since
>> it failed
>> to follow the style guidelines, and apr_ipsubnet_test since it's not
>> named
>> according to apr conventions, so these are just two examples that this
>> code
>> isn't well thought out. But let me break this down and ask which
>> fix(es) are
>> appropriate, we'll start at the segfaulting test itself...
>>
>> static void test_interesting_subnets(abts_case *tc, void *data)
>> {
>> struct {
>> const char *ipstr, *mask;
>> int family;
>> char *in_subnet, *not_in_subnet;
>> } testcases[] =
>> {
>> {"9.67", NULL, APR_INET, "9.67.113.15", "10.1.2.3"}
>> [...]
>> #if APR_HAVE_IPV6
>> [...]
>> ,{"3FFE:8160::", "28", APR_INET6, "3ffE:816e:abcd:1234::1",
>> "3ffe:8170::1"}
>>
>> [All of the tests above work on win32, with ipv6 compiled but without
>> ipv6 configured.
>> The problems lurk in the final two tests... note that these IP's
>> aren't even legal
>> IPv6 addresses...]
>>
>> ,{"127.0.0.1", NULL, APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
>> ,{"127.0.0.1", "8", APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
>> #endif /* IPV6 */
>> };
>> apr_ipsubnet_t *ipsub;
>> apr_sockaddr_t *sa;
>> apr_status_t rv;
>> int i, rc;
>>
>> for (i = 0; i < sizeof testcases / sizeof testcases[0]; i++) {
>> rv = apr_ipsubnet_create(&ipsub, testcases[i].ipstr,
>> testcases[i].mask, p);
>> ABTS_TRUE(tc, rv == APR_SUCCESS);
>> rv = apr_sockaddr_info_get(&sa, testcases[i].in_subnet,
>> testcases[i].family, 0, 0, p);
>>
>> [In the problem cases, apr_sockaddr_info_get returns SUCCESS, sa ==
>> NULL and
>> it's obviously related to how getaddrinfo works on win32, but as we'll
>> examine
>> in a moment - any platform might just return an sa of NULL with or
>> without an
>> error code. SO, ANY platform could explode...]
>>
>> ABTS_TRUE(tc, rv == APR_SUCCESS);
>> rc = apr_ipsubnet_test(ipsub, sa);
>>
>> This crashes, since sa-> is immediately dereferenced, in
>> apr_ipsubnet_test,
>> and would continue to crash on any number of platforms if they
>> encounter problems
>> with the test examples. Crashing is a totally unacceptable reaction
>> and prevents
>> us from seeing what other issues might exist (and be interrelated).
>>
>> First question of the test, should we dodge the apr_ipsubnet_test
>> above if we
>> see a non-success rv? or if we see a null sa? or never dodge it, but
>> accept NULL
>> to the apr_ipsubnet_test and return nomatch?
>>
>> Now my question below is; which patches should be applied? I'll break
>> them down
>> individually...
>>
>>
>> Index: network_io/unix/sockaddr.c
>> ===================================================================
>> --- network_io/unix/sockaddr.c (revision 584444)
>> +++ network_io/unix/sockaddr.c (working copy)
>> @@ -346,12 +346,18 @@
>> }
>> error = getaddrinfo(hostname, servname, &hints, &ai_list);
>> #ifdef HAVE_GAI_ADDRCONFIG
>> - if (error == EAI_BADFLAGS && family == APR_UNSPEC) {
>> + if ((!error && !ai_list)
>> + || (error == EAI_BADFLAGS && family == APR_UNSPEC)) {
>> +#else
>> + if (!error && !ai_list) {
>> +#endif
>> /* Retry with no flags if AI_ADDRCONFIG was rejected. */
>> hints.ai_flags = 0;
>> error = getaddrinfo(hostname, servname, &hints, &ai_list);
>>
>> [We anticipated that flags could cause a problem and we fall back in the
>> case that we were told our flags are bad, but do we also want to fall
>> back
>> and try without flags, if we encounter zero results for ai_list?]
>>
>>
>> + if (!error && !ai_list) {
>> + return EAI_NONAME + APR_OS_START_EAIERR;
>> + }
>> }
>> -#endif
>>
>> [The question comes down to, must we respond with an error if we have
>> zero
>> ai_list members, or is APR_SUCCESS plus zero ai_list elements an
>> anticipated
>> situation that the caller is expected to address?]
>>
>> @@ -944,10 +947,10 @@
>>
>> APR_DECLARE(int) apr_ipsubnet_test(apr_ipsubnet_t *ipsub,
>> apr_sockaddr_t *sa)
>> {
>> + if (!sa)
>> + return 0; /* no match (idiots) */
>> +
>> #if APR_HAVE_IPV6
>> - /* XXX This line will segv on Win32 build with APR_HAVE_IPV6,
>> - * but without the IPV6 drivers installed.
>> - */
>> if (sa->sa.sin.sin_family == AF_INET) {
>>
>> [Do we need to surpress idiocy such as the example use case, by
>> guarding against
>> such NULL sa's here in the apr_ipsubnet_test?]
>>
>> Comments please, from those of you more familiar with getaddrinfo than
>> I am?
>
>
Re-asking for feedback: testipsub bogosty centered on getaddrinfo
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Colm was correct; the addresses that an ipv6-kernel-sans-ipv6-driver can't
swallow on win32 are the IPv4-dotted-IPv6-prefixed formats. But it doesn't
fail gracefully, it just returns success and no elts in the list, which will
cause us to return NULL for the resulting sa, and crash soon afterwards.
Do we
* anticipate NULL sa's? (Seems silly)
* emit an error? Which one is appropriate for "i just don't understand,
but it doesn't mean your args were wrong"? Preferably something from
the EAI family that MS *should* have returned.
Bill
William A. Rowe, Jr. wrote:
> Trying do decide who's at fault here, I tend to blame the test since it
> failed
> to follow the style guidelines, and apr_ipsubnet_test since it's not named
> according to apr conventions, so these are just two examples that this code
> isn't well thought out. But let me break this down and ask which
> fix(es) are
> appropriate, we'll start at the segfaulting test itself...
>
> static void test_interesting_subnets(abts_case *tc, void *data)
> {
> struct {
> const char *ipstr, *mask;
> int family;
> char *in_subnet, *not_in_subnet;
> } testcases[] =
> {
> {"9.67", NULL, APR_INET, "9.67.113.15", "10.1.2.3"}
> [...]
> #if APR_HAVE_IPV6
> [...]
> ,{"3FFE:8160::", "28", APR_INET6, "3ffE:816e:abcd:1234::1",
> "3ffe:8170::1"}
>
> [All of the tests above work on win32, with ipv6 compiled but without
> ipv6 configured.
> The problems lurk in the final two tests... note that these IP's aren't
> even legal
> IPv6 addresses...]
>
> ,{"127.0.0.1", NULL, APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
> ,{"127.0.0.1", "8", APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
> #endif /* IPV6 */
> };
> apr_ipsubnet_t *ipsub;
> apr_sockaddr_t *sa;
> apr_status_t rv;
> int i, rc;
>
> for (i = 0; i < sizeof testcases / sizeof testcases[0]; i++) {
> rv = apr_ipsubnet_create(&ipsub, testcases[i].ipstr,
> testcases[i].mask, p);
> ABTS_TRUE(tc, rv == APR_SUCCESS);
> rv = apr_sockaddr_info_get(&sa, testcases[i].in_subnet,
> testcases[i].family, 0, 0, p);
>
> [In the problem cases, apr_sockaddr_info_get returns SUCCESS, sa == NULL
> and
> it's obviously related to how getaddrinfo works on win32, but as we'll
> examine
> in a moment - any platform might just return an sa of NULL with or
> without an
> error code. SO, ANY platform could explode...]
>
> ABTS_TRUE(tc, rv == APR_SUCCESS);
> rc = apr_ipsubnet_test(ipsub, sa);
>
> This crashes, since sa-> is immediately dereferenced, in apr_ipsubnet_test,
> and would continue to crash on any number of platforms if they encounter
> problems
> with the test examples. Crashing is a totally unacceptable reaction and
> prevents
> us from seeing what other issues might exist (and be interrelated).
>
> First question of the test, should we dodge the apr_ipsubnet_test above
> if we
> see a non-success rv? or if we see a null sa? or never dodge it, but
> accept NULL
> to the apr_ipsubnet_test and return nomatch?
>
> Now my question below is; which patches should be applied? I'll break
> them down
> individually...
>
>
> Index: network_io/unix/sockaddr.c
> ===================================================================
> --- network_io/unix/sockaddr.c (revision 584444)
> +++ network_io/unix/sockaddr.c (working copy)
> @@ -346,12 +346,18 @@
> }
> error = getaddrinfo(hostname, servname, &hints, &ai_list);
> #ifdef HAVE_GAI_ADDRCONFIG
> - if (error == EAI_BADFLAGS && family == APR_UNSPEC) {
> + if ((!error && !ai_list)
> + || (error == EAI_BADFLAGS && family == APR_UNSPEC)) {
> +#else
> + if (!error && !ai_list) {
> +#endif
> /* Retry with no flags if AI_ADDRCONFIG was rejected. */
> hints.ai_flags = 0;
> error = getaddrinfo(hostname, servname, &hints, &ai_list);
>
> [We anticipated that flags could cause a problem and we fall back in the
> case that we were told our flags are bad, but do we also want to fall back
> and try without flags, if we encounter zero results for ai_list?]
>
>
> + if (!error && !ai_list) {
> + return EAI_NONAME + APR_OS_START_EAIERR;
> + }
> }
> -#endif
>
> [The question comes down to, must we respond with an error if we have zero
> ai_list members, or is APR_SUCCESS plus zero ai_list elements an
> anticipated
> situation that the caller is expected to address?]
>
> @@ -944,10 +947,10 @@
>
> APR_DECLARE(int) apr_ipsubnet_test(apr_ipsubnet_t *ipsub,
> apr_sockaddr_t *sa)
> {
> + if (!sa)
> + return 0; /* no match (idiots) */
> +
> #if APR_HAVE_IPV6
> - /* XXX This line will segv on Win32 build with APR_HAVE_IPV6,
> - * but without the IPV6 drivers installed.
> - */
> if (sa->sa.sin.sin_family == AF_INET) {
>
> [Do we need to surpress idiocy such as the example use case, by guarding
> against
> such NULL sa's here in the apr_ipsubnet_test?]
>
> Comments please, from those of you more familiar with getaddrinfo than I
> am?
Re: testipsub bogosty centered on getaddrinfo
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Colm MacCarthaigh wrote:
>
> Which two IP's arn't even legal IPv6 addresses?
>
>> ,{"127.0.0.1", NULL, APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
>> ,{"127.0.0.1", "8", APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
>
> These two? They are legal, ::ffff:127.0.0.1 is an IPv4-mapped IPv6
> address representing localhost and fe80::1 is a valid link-scoped
> address.
"127.0.0.1"
Either "::127.0.0.1" or "::FFFF:127.0.0.1" would be valid, of course.
Bill
Re: testipsub bogosty centered on getaddrinfo
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Sat, Oct 13, 2007 at 06:41:41PM -0500, William A. Rowe, Jr. wrote:
> ,{"3FFE:8160::", "28", APR_INET6, "3ffE:816e:abcd:1234::1",
> "3ffe:8170::1"}
>
> [All of the tests above work on win32, with ipv6 compiled but without ipv6
> configured.
> The problems lurk in the final two tests... note that these IP's aren't
> even legal
> IPv6 addresses...]
Which two IP's arn't even legal IPv6 addresses?
> ,{"127.0.0.1", NULL, APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
> ,{"127.0.0.1", "8", APR_INET6, "::ffff:127.0.0.1", "fe80::1"}
These two? They are legal, ::ffff:127.0.0.1 is an IPv4-mapped IPv6
address representing localhost and fe80::1 is a valid link-scoped
address.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net