apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: Re-asking for feedback: testipsub bogosty centered on getaddrinfo
Date Tue, 30 Oct 2007 05:04:11 GMT
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?
> 
> 


Mime
View raw message