MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:August 1 2009 3:17am
Subject:Re: bzr commit into mysql-5.4 branch (alik:2804) Bug#45584
View as plain text  
On 7/28/09 10:10 AM, Alexander Nozdrin wrote:
> #At file:///mnt/raid/alik/MySQL/bzr/bug38247/azalea-bf-bug45584/ based on
> revid:alik@stripped
>
>   2804 Alexander Nozdrin	2009-07-28
>        Fix for Bug#45584 (Host name cache does not work as a cache).
>
>        The problem is described in the bug report.
>
>        The solution is the following:
>
>          - Make hostname cache key type of (char *);
>
>          - Use string representation of normalized IPv6 addresses
>            as hostname cache keys when IPv6 is supported.
>
>            Use string representation of normalized IPv4 addresses
>            as hostname cache keys when IPv6 is not supported.
>
>          - Use only the host part of client address for hostname
>            cache keys;
>
>          - Actually resolve IP addresses to hostnames,
>            not to IP strings;
>
>          - Minimal supported Windows version has been changed to Windows XP.
>            We don't support Windows 2000 for the newer versions any more.
>
>            Windows XP has IPv6 support, so declaring it minimal supported
>            version removes much of "windows portability hassle".
>       @ configure.in
>          HAVE_STRUCT_IN6_ADDR will be defined when IPv6 is supported.
>       @ sql/mysql_priv.h
>          Change the ip_to_hostname() prototype:
>            - Make it more descriptive;
>            - Provide a way to return error status;
>       @ sql/sql_connect.cc
>          Handle errors from ip_to_hostname().
>
>      modified:
>        configure.in
>        include/config-win.h
>        sql/hostname.cc
>        sql/mysql_priv.h
>        sql/sql_connect.cc
> === modified file 'configure.in'
> --- a/configure.in	2009-06-17 07:30:19 +0000
> +++ b/configure.in	2009-07-28 13:09:58 +0000
> @@ -876,6 +876,24 @@ then
>   fi
>
>   #--------------------------------------------------------------------
> +# Check for IPv6 support
> +#--------------------------------------------------------------------
> +
> +AC_CHECK_HEADERS(netinet/in6.h)
> +
> +AC_CHECK_TYPES([struct in6_addr], [], [],
> +[[#ifdef WIN32
> +#include<winsock2.h>
> +#else
> +#include<sys/types.h>
> +#include<netinet/in.h>
> +#include<sys/socket.h>
> +#endif
> +#ifdef HAVE_NETINET_IN6_H
> +#include<netinet/in6.h>
> +#endif]])

Attached to this e-mail is a patch to augment the IPv6 document. Please 
consider adopting it and using the HAVE_IPV6 macro to test for IPv6 
support. If something goes wrong with the IPv6 code, this switch is 
helpful to just compile out the whole stuff.

Add to CMakeLists.txt:

ADD_DEFINITIONS(-DHAVE_IPV6)

as to enable IPv6 support on Windows. Makes easy to disable if necessary.

[..]

>
> === modified file 'sql/hostname.cc'
> --- a/sql/hostname.cc	2009-01-27 02:08:48 +0000
> +++ b/sql/hostname.cc	2009-07-28 13:09:58 +0000
> @@ -18,10 +18,10 @@
>     @file
>
>     @brief
> -  Get hostname for an IP.
> +  Get hostname for an IP address.
>
> -    Hostnames are checked with reverse name lookup and
> -    checked that they doesn't resemble an ip.
> +  Hostnames are checked with reverse name lookup and checked that they
> +  doesn't resemble an IP address.
>   */
>
>   #include "mysql_priv.h"
> @@ -40,12 +40,228 @@ extern "C" {					// Because of SCO 3.2V4
>   }
>   #endif
>
> -class host_entry :public hash_filo_element
> +#ifdef __WIN__
> +#define HAVE_STRUCT_IN6_ADDR
> +#endif /* __WIN__ */
> +
> +/*
> +  HOST_ENTRY_KEY_SIZE -- size of IP address string in the hash cache. The
> +  system constant NI_MAXHOST could be used here. However, it means at least
> +  1024 bytes per IP, which seems to be quite big.

NI_MAXHOST represents the maximum length of a fully-qualified domain 
name. Hence, we shouldn't be using it at all :)

> +  Since IP address string is created by our function get_ip_string(), we
> +  can reduce the space.  get_ip_string() returns a hexadecimal
> +  respresentation (1111:2222:...:8888) in case of IPv6 and the standard

typo: respresentation

> +  representation (111.222.333.444) in case of IPv4, which means 39 bytes at
> +  most. So, we need 40 bytes for storing IP address string including
> +  trailing zero.
> +*/
> +
> +#define HOST_ENTRY_KEY_SIZE 40
> +
> +/************************************************************************/
> +
> +/*
> +  When this code was written there were issues with winsock in pusbuild,
> +  this constant is in this place for this reason.
> +*/
> +
> +#ifdef EAI_NODATA
> +    const int MY_NONAME_ERR_CODE= EAI_NODATA;
> +#else
> +    const int MY_NONAME_ERR_CODE= EAI_NONAME;
> +#endif

Given Frazer's comment and that the documented result in this case would 
be EAI_NONAME, perhaps it would be best to check for both conditions?

Something like:

#if defined(EAI_NONAME) && defined(EAI_NODATA)
#define IS_HOSTNAME_UNKNOWN(result)   \
          (result == EAI_NONAME || result == EAI_NODATA)
#elif defined (EAI_NONAME)
#define IS_HOSTNAME_UNKNOWN(result)   \
          (result == EAI_NONAME)
#elif defined (EAI_NODATA)
#define IS_HOSTNAME_UNKNOWN(result)   \
          (result == EAI_NODATA)
#else
#error EAI_NONAME and EAI_NODATA are not available
#endif

if (IS_HOSTNAME_UNKNOWN(err_code))
{
    ...
}

or how about just dropping this altogether? This was intended for 
Windows and we are increasing the minimum supported version...

> +/************************************************************************/
> +
> +/**
> +  Get the string representation for IP address. IPv6 and IPv4 addresses are

typo: "for an IP address"

> +  supported. The function is needed because getnameinfo() is known to be
> +  buggy in some circumstances. Actually, this is a basic replacement for
> +  getnameinfo() called with the NI_NUMERICHOST flag. Only the hostname part is
> +  dumped (the port part is omitted).
> +*/
> +
> +static void get_ip_string(const struct sockaddr *ip,
> +                          char *ip_str, int ip_str_size)
> +{
> +  switch (ip->sa_family) {
> +  case AF_INET:
> +  {
> +    struct in_addr *ip4=&((struct sockaddr_in *) ip)->sin_addr;
> +    uint32 ip4_int32= ntohl(ip4->s_addr); // XXX

Drop XXX? Looks good to me.

> +    uint8 *ip4_int8= (uint8 *)&ip4_int32;
> +
> +    int n= my_snprintf(ip_str, ip_str_size, "%d.%d.%d.%d",
> +                       ip4_int8[0], ip4_int8[1], ip4_int8[2], ip4_int8[3]);

A int type format is specified (%d), yet the type is a uint8. Could read 
past the end of the original integer.

Perhaps it would be better to "unroll" both procedures (AF_INET and 
AF_INET6) and do the conversion manually without using my_snprintf?

> +    DBUG_ASSERT(n < ip_str_size);
> +    break;
> +  }
> +
> +#ifdef HAVE_STRUCT_IN6_ADDR
> +  case AF_INET6:
> +  {
> +    struct in6_addr *ip6=&((struct sockaddr_in6 *) ip)->sin6_addr;
> +    uint16 *ip6_int16= (uint16 *) ip6->s6_addr;
> +
> +    int n= my_snprintf(ip_str, ip_str_size,
> +                       "%04X:%04X:%04X:%04X:%04X:%04X:%04X:%04X",
> +                       ntohs(ip6_int16[0]), ntohs(ip6_int16[1]),
> +                       ntohs(ip6_int16[2]), ntohs(ip6_int16[3]),
> +                       ntohs(ip6_int16[4]), ntohs(ip6_int16[5]),
> +                       ntohs(ip6_int16[6]), ntohs(ip6_int16[7]));
> +
> +    DBUG_ASSERT(n<  ip_str_size);
> +    break;
> +  }
> +#endif /* HAVE_STRUCT_IN6_ADDR */
> +
> +  default:
> +    DBUG_ASSERT(FALSE);
> +  }
> +}
> +
> +/**
> +  Get key value for IP address. Key value is a string representation of
> +  normalized IP address.
> +
> +  When IPv4 and IPv6 are both used in the network stack, or in the network
> +  path between a client and a server, a client can have different apparent
> +  IP addresses, based on the exact route taken.
> +
> +  This function normalize the client IP representation, so it's suitable to
> +  use as a key for searches.
> +
> +  Transformations are implemented as follows:
> +  - IPv4 a.b.c.d -->  IPv6 mapped IPv4 ::ffff:a.b.c.d
> +  - IPv6 compat IPv4 ::a.b.c.d -->  IPv6 mapped IPv4 ::ffff:a.b.c.d
> +  - IPv6 -->  IPv6
> +
> +  If IPv6 is not available at compile-time, IPv4 form is used.
> +
> +  @param [in]   ip      IP address
> +  @param [out]  ip_key  Key for the given IP value
> +
> +  @note According to RFC3493 the only specified member of the in6_addr
> +  structure is s6_addr.
> +
> +  @note It is possible to call hostname_cache_get_key() with zeroed IP
> +  address (ip->sa_family == 0). In this case hostname_cache_get_key()
> +  returns TRUE (error status).

In which cases is this possible?

> +  @return Error status.
> +  @retval FALSE Success
> +  @retval TRUE Error
> +*/
> +
> +static bool hostname_cache_get_key(const struct sockaddr *ip, char *ip_key)
> +{
> +  const struct sockaddr *ip_to_generate_key= ip;
> +
> +  if (ip->sa_family == 0)
> +    return TRUE; /* IP address is not set. */

Perhaps it should be:

if (ip->sa_family != AF_INET || ip->sa_family != AF_INET6)

But, hum.. this function gets called for AF_UNIX sockets?

> +
> +#ifdef HAVE_STRUCT_IN6_ADDR
> +  /* Prepare normalized IP address. */
> +

[..]

> -  char	 *hostname;
> +  /**
> +    Client IP address. This is the key used with the hash table.
> +
> +    The client IP address is always expressed in IPv6, even when the
> +    network IPv6 stack is not present.
> +
> +    This IP address is never used to connect to a socket.
> +  */
> +  char ip[HOST_ENTRY_KEY_SIZE];
> +
> +  /**
> +    Number of errors during handshake phase from the IP address.
> +  */
> +  uint connect_errors;
> +
> +  /**
> +    One of host names for the IP address. May be NULL.

typo: One of the host names

> +  */
> +  const char *hostname;
>   };
>
>   static hash_filo *hostname_cache;
> @@ -57,13 +273,15 @@ void hostname_cache_refresh()
>
>   bool hostname_cache_init()
>   {
> -  host_entry tmp;
> -  uint offset= (uint) ((char*) (&tmp.ip) - (char*)&tmp);
> -  if (!(hostname_cache=new hash_filo(HOST_CACHE_SIZE, offset,
> -				     sizeof(struct sockaddr_storage),NULL,
> -				     (my_hash_free_key) free,
> -				&my_charset_bin)))
> +  Host_entry tmp;
> +  uint key_offset= (uint) ((char*) (&tmp.ip) - (char*)&tmp);
> +
> +  if (!(hostname_cache= new hash_filo(HOST_CACHE_SIZE,
> +                                      key_offset, HOST_ENTRY_KEY_SIZE,
> +                                      NULL, (my_hash_free_key) free,
> +&my_charset_bin)))
>       return 1;
> +
>     hostname_cache->clear();
>
>     return 0;
> @@ -71,190 +289,461 @@ bool hostname_cache_init()
>
>   void hostname_cache_free()
>   {
> -  if (hostname_cache)
> -  {
> -    delete hostname_cache;
> -    hostname_cache= 0;
> -  }
> +  delete hostname_cache;
> +  hostname_cache= NULL;
>   }
>
> -static void add_hostname(struct sockaddr_storage *in, const char *name)
> +static inline Host_entry *hostname_cache_search(const char *ip)
>   {
> -  if (!(specialflag&  SPECIAL_NO_HOST_CACHE))
> +  return (Host_entry *) hostname_cache->search((uchar *) ip, 0);
> +}
> +
> +static bool add_hostname_impl(const char *ip, const char *hostname)
> +{
> +  if (hostname_cache_search(ip))
> +    return FALSE;
> +
> +  uint hostname_length= hostname ? (uint) strlen(hostname) : 0;

Hum, i think we safely use size_t. Also:

size_t hostname_length= hostname? strlen(hostname) + 1 : 0;

So that a extra byte is only allocated when necessary.

> +  Host_entry *entry= (Host_entry *) malloc(sizeof (Host_entry) +
> +                                           hostname_length + 1);
> +
> +  if (!entry)
> +    return TRUE;
> +
> +  char *hostname_copy;
> +
> +  memcpy_fixed(&entry->ip, ip, HOST_ENTRY_KEY_SIZE);
> +
> +  if (hostname_length)
>     {
> -    pthread_mutex_lock(&hostname_cache->lock);
> -    host_entry *entry;
> -    if (!(entry=(host_entry*) hostname_cache->search((uchar*) in, 0)))
> -    {
> -      uint length=name ? (uint) strlen(name) : 0;
> +    hostname_copy= (char *) (entry + 1);
> +    memcpy(hostname_copy, hostname, hostname_length + 1);
>
> -      if ((entry=(host_entry*) malloc(sizeof(host_entry)+length+1)))
> -      {
> -	char *new_name;
> -	memcpy_fixed(&entry->ip, in, sizeof(struct sockaddr_storage));
> -	if (length)
> -	  memcpy(new_name= (char *) (entry+1), name, length+1);
> -	else
> -	  new_name=0;
> -	entry->hostname=new_name;
> -	entry->errors=0;
> -	(void) hostname_cache->add(entry);
> -      }
> -    }
> -    pthread_mutex_unlock(&hostname_cache->lock);
> +    DBUG_PRINT("info", ("Adding '%s' ->  '%s' to the hostname cache...'",
> +                        (const char *) ip,
> +                        (const char *) hostname_copy));
>     }
> +  else
> +  {
> +    hostname_copy= NULL;
> +
> +    DBUG_PRINT("info", ("Adding '%s' ->  NULL to the hostname cache...'",
> +                        (const char *) ip));
> +  }
> +
> +  entry->hostname= hostname_copy;
> +  entry->connect_errors= 0;
> +
> +  return hostname_cache->add(entry);
>   }
>

[..]

Otherwise looks good. I would like to take a look at the last patch 
before approving. Thanks for working on this!

Regards,

-- Davi Arnaut

=== modified file 'configure.in'
--- configure.in	2009-07-28 14:16:37 +0000
+++ configure.in	2009-08-01 01:45:32 +0000
@@ -877,6 +877,37 @@ then
 fi
 
 #--------------------------------------------------------------------
+# Check for IPv6 support
+#--------------------------------------------------------------------
+
+AC_CHECK_HEADERS(netinet/in6.h)
+
+AC_CHECK_TYPES([struct sockaddr_in6, struct in6_addr],
+  [have_in6_types=yes], [have_in6_types=no],
+  [[#ifdef WIN32
+    #include <winsock2.h>
+    #else
+    #include <sys/types.h>
+    #include <netinet/in.h>
+    #include <sys/socket.h>
+    #endif
+    #ifdef HAVE_NETINET_IN6_H
+    #include <netinet/in6.h>
+    #endif]])
+
+AC_MSG_CHECKING([for IPv6 support])
+AC_ARG_ENABLE(ipv6,
+  AS_HELP_STRING([--disable-ipv6], [Disable support for IPv6 networking]),
+  [disable_ipv6=yes], [disable_ipv6=no])
+
+if test x"$disable_ipv6" = xyes -o x"$have_in6_types" = xno; then
+  AC_MSG_RESULT([no])
+else
+  AC_DEFINE([HAVE_IPV6], [1], [Define if IPv6 networking support is present])
+  AC_MSG_RESULT([yes])
+fi
+
+#--------------------------------------------------------------------
 # Check for TCP wrapper support
 #--------------------------------------------------------------------
 



Thread
bzr commit into mysql-5.4 branch (alik:2804) Bug#45584Alexander Nozdrin28 Jul
  • Re: bzr commit into mysql-5.4 branch (alik:2804) Bug#45584Frazer Clement28 Jul
  • Re: bzr commit into mysql-5.4 branch (alik:2804) Bug#45584Frazer Clement29 Jul
  • Re: bzr commit into mysql-5.4 branch (alik:2804) Bug#45584Davi Arnaut1 Aug