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 testipsub bogosty centered on getaddrinfo
Date Sat, 13 Oct 2007 23:41:41 GMT
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

Mime
View raw message