List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:June 25 2009 10:28pm
Subject:Re: bzr commit into mysql-5.4 branch (alik:2804) Bug#45584
View as plain text  
Hi Alexander,

On 6/24/09 11:19 AM, Alexander Nozdrin wrote:
> #At file:///mnt/raid/alik/MySQL/bzr/bug38247/azalea-bf-bug45584/ based on
> revid:alik@stripped
>
>   2804 Alexander Nozdrin	2009-06-24
>        Fix for Bug#45584 (Host name cache does not work as a cache).

I guess the final patch will have detailed comments :-)

>      modified:
>        sql/hostname.cc
>        sql/mysql_priv.h
>        sql/sql_connect.cc
> === modified file 'sql/hostname.cc'
> --- a/sql/hostname.cc	2009-01-27 02:08:48 +0000
> +++ b/sql/hostname.cc	2009-06-24 14:19:40 +0000
> @@ -21,7 +21,7 @@
>     Get hostname for an IP.
>
>       Hostnames are checked with reverse name lookup and
> -    checked that they doesn't resemble an ip.
> +    checked that they doesn't resemble an IP.
>   */
>
>   #include "mysql_priv.h"
> @@ -40,12 +40,220 @@ extern "C" {					// Because of SCO 3.2V4
>   }
>   #endif
>
> +#define HOST_ENTRY_KEY_SIZE NI_MAXHOST
> +
> +/*************************************************************************
> +  On Windows IN6_IS_ADDR_LOOPBACK, IN6_IS_ADDR_V4MAPPED,
> +  IN6_IS_ADDR_V4COMPAT are defined starting from Windows XP. The MySQL
> +  server is supported starting from Windows 2000, where they are not
> +  available. That's why we have to implement our own replacements.
> +**************************************************************************/
> +
> +static inline bool my_is_ip6_loopback(const struct in6_addr *ip6);
> +static inline bool my_is_ip6_v4_mapped(const struct in6_addr *ip6);
> +static inline bool my_is_ip6_v4_compat(const struct in6_addr *ip6);

Why is this necessary?

> +#if defined(__WIN__)&&  defined(_WIN32_WINNT)&&  _WIN32_WINNT<=
> 0x0500
> +
> +static inline bool my_is_ip6_loopback(const struct in6_addr *ip6)
> +{
> +  return ip6->s6_words[0] == 0&&
> +         ip6->s6_words[1] == 0&&
> +         ip6->s6_words[2] == 0&&
> +         ip6->s6_words[3] == 0&&
> +         ip6->s6_words[4] == 0&&
> +         ip6->s6_words[5] == 0&&
> +         ip6->s6_words[6] == 0&&
> +         ip6->s6_words[7] == 0x0100;
> +}
> +
> +static inline bool my_is_ip6_v4_mapped(const struct in6_addr *ip6)
> +{
> +  return ip6->s6_words[0] == 0&&
> +         ip6->s6_words[1] == 0&&
> +         ip6->s6_words[2] == 0&&
> +         ip6->s6_words[3] == 0&&
> +         ip6->s6_words[4] == 0&&
> +         ip6->s6_words[5] == 0xffff;
> +}
> +
> +static inline bool my_is_ip6_v4_compat(const struct in6_addr *ip6)
> +{
> +    return ip6->s6_words[0] == 0&&
> +           ip6->s6_words[1] == 0&&
> +           ip6->s6_words[2] == 0&&
> +           ip6->s6_words[3] == 0&&
> +           ip6->s6_words[4] == 0&&
> +           ip6->s6_words[5] == 0&&
> +           (ip6->s6_words[6] != 0 ||
> +            ip6->s6_addr[14] != 0 ||
> +            ip6->s6_addr[15] != 0&&  ip6->s6_addr[15] != 1);
> +}
> +
> +#else
> +
> +static inline bool my_is_ip6_loopback(const struct in6_addr *ip6)
> +{
> +  return IN6_IS_ADDR_LOOPBACK(ip6);
> +}
> +
> +static inline bool my_is_ip6_v4_mapped(const struct in6_addr *ip6)
> +{
> +  return IN6_IS_ADDR_V4MAPPED(ip6);
> +}
> +
> +static inline bool my_is_ip6_v4_compat(const struct in6_addr *ip6)
> +{
> +    return IN6_IS_ADDR_V4COMPAT(ip6);
> +}
> +
> +#endif

Also, do we plan to support systems that don't have IPv6?

> +/************************************************************************/
> +
> +/*
> +  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
> +
> +/************************************************************************/
> +
> +/**
> +  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 supported on the system, we could use the IPv4.

> +  @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.
> +
> +  @return Error status.
> +  @retval FALSE Success
> +  @retval TRUE Error
> +*/
> +
> +static bool hostname_cache_get_key(const struct sockaddr *ip, char *ip_key)
> +{
> +  /* Prepare normalized IP address. */
> +
> +  struct sockaddr_storage norm_ip;
> +  struct in6_addr *norm_ip6=&((sockaddr_in6 *)&norm_ip)->sin6_addr;
> +  uint32 *norm_ip6_int32= (uint32 *) norm_ip6->s6_addr;
> +
> +  bzero(&norm_ip, sizeof (sockaddr_storage));

memset.

> +  switch (ip->sa_family){

Space after ).

> +  case AF_INET:
> +  {
> +    struct in_addr *ip4=&((struct sockaddr_in *) ip)->sin_addr;
> +
> +    norm_ip6_int32[0]= 0;
> +    norm_ip6_int32[1]= 0;
> +    norm_ip6_int32[2]= htonl(0xffff);
> +    norm_ip6_int32[3]= ip4->s_addr; /* in net byte order */
> +
> +    DBUG_ASSERT(my_is_ip6_v4_mapped(norm_ip6));
> +    break;

OK.

> +  }
> +
> +  case AF_INET6:
> +  {
> +    struct in6_addr *ip6=&((struct sockaddr_in6 *) ip)->sin6_addr;
> +    uint32 *ip6_int32= (uint32 *) ip6->s6_addr;
> +
> +    if (my_is_ip6_v4_compat(ip6))
> +    {
> +      norm_ip6_int32[0]= 0;
> +      norm_ip6_int32[1]= 0;
> +      norm_ip6_int32[2]= htonl(0xffff);
> +      norm_ip6_int32[3]= ip6_int32[3]; /* in net byte order */
> +      DBUG_ASSERT(my_is_ip6_v4_mapped(norm_ip6));
> +    }
> +    else
> +    {
> +      /* All in net byte order */
> +      norm_ip6_int32[0]= ip6_int32[0];
> +      norm_ip6_int32[1]= ip6_int32[1];
> +      norm_ip6_int32[2]= ip6_int32[2];
> +      norm_ip6_int32[3]= ip6_int32[3];

memcpy ?

> +    }
> +
> +    break;
> +  }
> +
> +  default:
> +    DBUG_ASSERT(FALSE);
> +    break;
> +  }

We could ifdef out the above to support cases where IPv6 infrastructure 
is not available.

> +  /*
> +    Zero all bytes of the key, because it's not just 0-terminated string.
> +    All bytes are taken into account during hash search.
> +  */
> +
> +  bzero(ip_key, HOST_ENTRY_KEY_SIZE);

memset

> +  /* Get numeric representation of the normalized IP address. */
> +
> +  norm_ip.ss_family= AF_INET6;
> +  return getnameinfo((struct sockaddr *)&norm_ip, sizeof (norm_ip),
> +                     ip_key, HOST_ENTRY_KEY_SIZE, NULL, 0,
> +                     NI_NUMERICHOST) != 0;
> +
> +}

OK.

> +/**
> +  An entry in the hostname hash table cache.
> +
> +  Host name cache does two things:
> +    - caches host names to save DNS look ups;
> +    - counts connect errors from IP.
> +
> +  Host name can be NULL (that means DNS look up failed), but connect errors
> +  still are counted.
> +*/
> +
>   class host_entry :public hash_filo_element
>   {
>   public:
> -  char	 ip[sizeof(struct sockaddr_storage)];
> -  uint	 errors;
> -  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.
> +  */
> +  char *hostname;

const char *hostname;

>   };
>
>   static hash_filo *hostname_cache;
> @@ -58,12 +266,14 @@ void hostname_cache_refresh()
>   bool hostname_cache_init()
>   {
>     host_entry tmp;
> -  uint offset= (uint) ((char*) (&tmp.ip) - (char*)&tmp);

As a side node, i'm not really sure if hash_filo_element can be 
considered a POD type because of the constructor...

> -  if (!(hostname_cache=new hash_filo(HOST_CACHE_SIZE, offset,
> -				     sizeof(struct sockaddr_storage),NULL,
> -				     (my_hash_free_key) free,
> -				&my_charset_bin)))
> +  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;
> @@ -74,187 +284,378 @@ void hostname_cache_free()
>     if (hostname_cache)
>     {
>       delete hostname_cache;
> -    hostname_cache= 0;
> +    hostname_cache= NULL;
>     }

Actually, no need for the if. The operand of delete may be 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 void add_hostname(const char *ip, const char *name)

name -> hostname

> +{
> +  if (specialflag & SPECIAL_NO_HOST_CACHE)
> +    return;

Check would be better on a higher level since we only invoke this 
function twice..

> +  pthread_mutex_lock(&hostname_cache->lock);
> +  host_entry *entry;
> +  if (!(entry= hostname_cache_search(ip)))
>     {
> -    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;
> +
> +    if ((entry=(host_entry*) malloc(sizeof(host_entry)+length+1)))
>       {
> -      uint length=name ? (uint) strlen(name) : 0;
> +      char *new_name;
> +      memcpy_fixed(&entry->ip, ip, HOST_ENTRY_KEY_SIZE);
> +      if (length)
> +      {
> +        memcpy(new_name= (char *) (entry+1), name, length+1);
>
> -      if ((entry=(host_entry*) malloc(sizeof(host_entry)+length+1)))
> +        DBUG_PRINT("info", ("Adding '%s' ->  '%s' to the hostname cache...'",
> +                            (const char *) ip,
> +                            (const char *) new_name));

Do we need the casts? There are quite a few occurrences..

> +      }
> +      else
>         {
> -	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);
> +        new_name=0;

0 -> NULL

> +
> +        DBUG_PRINT("info", ("Adding '%s' ->  NULL to the hostname cache...'",
> +                            (const char *) ip));
>         }
> +
> +      entry->hostname=new_name;
> +      entry->connect_errors=0;

Space after =.

Both (this and above) ok since diff thinks it's moving.. heh.

> +
> +      (void) hostname_cache->add(entry);
>       }
> -    pthread_mutex_unlock(&hostname_cache->lock);
>     }
> +  pthread_mutex_unlock(&hostname_cache->lock);
>   }
>
> -inline void add_wrong_ip(struct sockaddr_storage *in)
> +static inline void add_unresolved_hostname(const char *ip)
>   {
> -  add_hostname(in, NullS);
> +  add_hostname(ip, NULL);

IMHO, no need for this type of wrapper.

>   }
>
> -void inc_host_errors(struct sockaddr_storage *in)
> +void inc_host_errors(struct sockaddr_storage *ip)
>   {
> +  char key[HOST_ENTRY_KEY_SIZE];
> +
> +  if (hostname_cache_get_key((struct sockaddr *) ip, key))
> +  {
> +    DBUG_PRINT("error", ("Can not get key for IP address."));
> +    return; /* Ignore error. */
> +  }
> +
>     pthread_mutex_lock(&hostname_cache->lock);
> -  host_entry *entry;
>
> -  if ((entry=(host_entry*) hostname_cache->search((uchar*)in, 0)))
> -    entry->errors++;
> +  host_entry *entry= hostname_cache_search(key);
> +
> +  if (entry)
> +    entry->connect_errors++;
>
>     pthread_mutex_unlock(&hostname_cache->lock);
>   }
>
>
> -void reset_host_errors(struct sockaddr_storage *in)
> +void reset_host_errors(struct sockaddr_storage *ip)
>   {
> +  char key[HOST_ENTRY_KEY_SIZE];
> +
> +  if (hostname_cache_get_key((struct sockaddr *) ip, key))
> +  {
> +    DBUG_PRINT("error", ("Can not get key for IP address."));
> +    return; /* Ignore error. */
> +  }
> +
>     pthread_mutex_lock(&hostname_cache->lock);
> -  host_entry *entry;
>
> -  if ((entry=(host_entry*) hostname_cache->search((uchar*)in, 0)))
> -    entry->errors=0;
> +  host_entry *entry= hostname_cache_search(key);
> +
> +  if (entry)
> +    entry->connect_errors=0;
>
>     pthread_mutex_unlock(&hostname_cache->lock);
>   }
>
>
> -char *ip_to_hostname(struct sockaddr_storage *in, int addrLen, uint *errors)
> +static inline bool is_ip_loopback(const struct sockaddr *ip)
> +{
> +  switch (ip->sa_family){
> +  case AF_INET:
> +    {
> +      /* Check for IPv4 127.0.0.1. */
> +      struct in_addr *ip4_addr=&((struct sockaddr_in *) ip)->sin_addr;
> +      return ip4_addr->s_addr== INADDR_LOOPBACK;

Space before ==.

> +    }
> +  case AF_INET6:
> +    {
> +      /* Check for IPv6 ::1. */
> +      struct in6_addr *ip6_addr=&((struct sockaddr_in6 *) ip)->sin6_addr;
> +      return my_is_ip6_loopback(ip6_addr);
> +    }
> +  default:
> +    DBUG_ASSERT(FALSE);
> +    return FALSE;
> +  }
> +}
> +
> +static inline bool is_hostname_valid(const char *hostname)
>   {
> -  char *name;
> +  if (!my_isdigit(&my_charset_latin1, hostname[0]))
> +    return TRUE;
>
> -  struct addrinfo hints,*res_lst,*t_res;
> -  int gxi_error;
> -  char hostname_buff[NI_MAXHOST];
> +  const char *pos;
> +
> +  for (pos= hostname + 1; my_isdigit(&my_charset_latin1, *pos); pos++)
> +    ;
> +
> +  return *pos == '.';
> +}
> +
> +/**
> +  Resolve IP-address to host name.
> +
> +  This function does the following things:
> +    - resolves IP-address;
> +    - employs Forward Confirmed Reverse DNS technique to validate IP-address;
> +    - returns host name if IP-address is validated;
> +    - set value to out-variable connect_errors -- this variable represents the
> +      number of connection errors from the specified IP-address.
> +
> +  NOTE: connect_errors are counted (are supported) only for the clients
> +  where IP-address can be resolved and FCrDNS check is passed.
> +
> +  @param [in] ip_storage
> +  @param [out] hostname
> +  @param [out] connect_errors
> +
> +  @return Error status
> +  @retval FALSE Success
> +  @retval TRUE Error
> +
> +  The function does not set/report MySQL server error in case of failure.
> +  It's caller's responsibility to handle failures of this function
> +  properly.
> +*/
> +
> +bool ip_to_hostname(struct sockaddr_storage *ip_storage,
> +                    char **hostname, uint *connect_errors)
> +{
> +  const struct sockaddr *ip= (const sockaddr *) ip_storage;
> +  char ip_string[HOST_ENTRY_KEY_SIZE];
> +  int err_code;
>
> -  host_entry *entry;
>     DBUG_ENTER("ip_to_hostname");
> -  *errors=0;
>
> -  /* Historical comparison for 127.0.0.1 */
> -  gxi_error= getnameinfo((struct sockaddr *)in, addrLen,
> -                         hostname_buff, NI_MAXHOST,
> -                         NULL, 0, NI_NUMERICHOST);
> -  if (gxi_error)
> -  {
> -    DBUG_PRINT("error",("getnameinfo returned %d", gxi_error));
> -    DBUG_RETURN(0);
> -  }
> -  DBUG_PRINT("info",("resolved: %s", hostname_buff));
> +  /* Get hostname cache key for the IP address. */
>
> -  /* The next three compares are to solve historical solutions with localhost */
> -  if (!memcmp(hostname_buff, "127.0.0.1", sizeof("127.0.0.1")))
> +  if (hostname_cache_get_key(ip, ip_string))
>     {
> -    DBUG_RETURN((char *)my_localhost);
> +    DBUG_PRINT("error", ("Can not get key for IP address."));
> +    return TRUE;

return TRUE -> DBUG_RETURN(TRUE)

Hum, we won't accept connections if getnameinfo can't return a IP 
address... for example: http://tinyurl.com/l23dvx

Do we care? I'm inclined to not care, but we definitely print a warning.

>     }
> -  if (!memcmp(hostname_buff, "::ffff:127.0.0.1", sizeof("::ffff:127.0.0.1")))
> -  {
> -    DBUG_RETURN((char *)my_localhost);
> -  }
> -  if (!memcmp(hostname_buff, "::1", sizeof("::1")))
> +
> +  DBUG_PRINT("info", ("IP address: '%s'.", (const char *) ip_string));
> +
> +  /* Check if we have loopback address (127.0.0.1 or ::1). */
> +
> +  if (is_ip_loopback(ip))

Shouldn't we check this first?

>     {
> -    DBUG_RETURN((char *)my_localhost);
> +    DBUG_PRINT("info", ("IP address (%s) corresponds to loopback.",
> +                        (const char *) ip_string));
> +
> +    *connect_errors= 0; /* Do not count connect errors from localhost. */
> +    *hostname= (char *) my_localhost;
> +
> +    DBUG_RETURN(FALSE);
>     }
>
> -  /* Check first if we have name in cache */
> +  /* Check first if we have host name in the cache. */
> +
>     if (!(specialflag&  SPECIAL_NO_HOST_CACHE))
>     {
>       pthread_mutex_lock(&hostname_cache->lock);
> -    if ((entry= (host_entry*)hostname_cache->search((uchar *)&in, 0)))
> +
> +    host_entry *entry= hostname_cache_search(ip_string);
> +
> +    if (entry)
>       {
> +      *connect_errors= entry->connect_errors;
> +      *hostname= NULL;
> +
>         if (entry->hostname)
> -        name= my_strdup(entry->hostname, MYF(0));
> -      else
> -        name= NULL;
> +        *hostname= my_strdup(entry->hostname, MYF(0));
> +
> +      DBUG_PRINT("info",("IP (%s) has been found in the cache. "
> +                         "Hostname: '%s'; connect_errors: %d",
> +                         (const char *) ip_string,
> +                         (const char *) (*hostname? *hostname : "null"),
> +                         (int) *connect_errors));
>
> -      DBUG_PRINT("info",("cached data %s", name ? name : "null" ));
> -      *errors= entry->errors;
>         pthread_mutex_unlock(&hostname_cache->lock);
> -      DBUG_RETURN(name);
> +
> +      DBUG_RETURN(FALSE);
>       }
> +
>       pthread_mutex_unlock(&hostname_cache->lock);
>     }
>
> -  if (!(name= my_strdup(hostname_buff, MYF(0))))
> +  /* Resolve host name. */
> +
> +  char hostname_buffer[NI_MAXHOST];
> +
> +  DBUG_PRINT("info", ("Resolving '%s'...", (const char *) ip_string));
> +
> +  err_code= getnameinfo(ip, sizeof (sockaddr_storage),
> +                        hostname_buffer, NI_MAXHOST, NULL, 0, 0);
> +  if (err_code)
>     {
> -    DBUG_PRINT("error",("out of memory"));
> -    DBUG_RETURN(0);
> +    DBUG_PRINT("error",("getnameinfo returned %d", err_code));

We ought to print a warning for failures..

> +    DBUG_RETURN(TRUE);
>     }
>
> -  /* Don't accept hostnames that starts with digits because they may be
> -    false ip:s */
> -  if (my_isdigit(&my_charset_latin1, name[0]))
> +  DBUG_PRINT("info", ("IP '%s' resolved to '%s'.",
> +                      (const char *) ip_string,
> +                      (const char *) hostname_buffer));
> +
> +  /*
> +    Validate host name: we don't accept hostnames that starts with digits
> +    because they may be false IPs.
> +
> +    TODO: that check should be extended to support IPv6.
> +  */
> +
> +  if (!is_hostname_valid(hostname_buffer))
>     {
> -    char *pos;
> -    for (pos= name+1 ; my_isdigit(&my_charset_latin1, *pos); pos++) ;
> -    if (*pos == '.')
> -    {
> -      DBUG_PRINT("error",("mysqld doesn't accept hostnames that starts with a number
> followed by a '.'"));
> -      goto add_wrong_ip_and_return;
> -    }
> +    DBUG_PRINT("error", ("mysqld doesn't accept hostnames that starts "
> +                         "with a number followed by a '.'"));
> +
> +    add_unresolved_hostname(ip_string);
> +    *hostname= NULL;
> +    *connect_errors= 0; /* New IP added to the cache. */
> +    DBUG_RETURN(TRUE);
>     }
>
> +  /* Get IP-addresses for the resolved host name (FCrDNS technique). */
> +
> +  struct addrinfo hints;
> +  struct addrinfo *addr_info_list;
> +
>     bzero(&hints, sizeof (struct addrinfo));
>     hints.ai_flags= AI_PASSIVE;
> -  hints.ai_socktype= SOCK_STREAM;
> +  hints.ai_socktype= SOCK_STREAM;
>     hints.ai_family= AF_UNSPEC;
>
> -  gxi_error= getaddrinfo(hostname_buff, NULL,&hints,&res_lst);
> -  if (gxi_error)
> +  DBUG_PRINT("info", ("Getting IP addresses for hostname '%s'...",
> +                      (const char *) hostname_buffer));
> +
> +  err_code= getaddrinfo(hostname_buffer, NULL,&hints,&addr_info_list);
> +
> +  if (err_code == MY_NONAME_ERR_CODE)
>     {
>       /*
> -      Don't cache responses when the DSN server is down, as otherwise
> +      Don't cache responses when the DNS server is down, as otherwise
>         transient DNS failure may leave any number of clients (those
>         that attempted to connect during the outage) unable to connect
>         indefinitely.
>       */
> -    /*
> -      When this code was written there were issues with winsock in pusbuild,
> -      this define is in this place for this reason.
> -    */
> -    DBUG_PRINT("error",("getaddrinfo returned %d", gxi_error));
> -#ifdef EAI_NODATA
> -    if (gxi_error == EAI_NODATA )
> -#else
> -    if (gxi_error == EAI_NONAME )
> -#endif
> -      add_wrong_ip(in);
>
> -    my_free(name,MYF(0));
> -    DBUG_RETURN(0);
> +    add_unresolved_hostname(ip_string);
> +    *hostname= NULL;
> +    *connect_errors= 0; /* New IP added to the cache. */
> +    DBUG_RETURN(FALSE);
> +  }
> +  else if (err_code)
> +  {
> +    DBUG_PRINT("error", ("getaddrinfo() failed with error code %d.", err_code));
> +    DBUG_RETURN(TRUE);
>     }
>
> -  /* Check that 'getaddrinfo' returned the used ip */
> -  for (t_res= res_lst; t_res; t_res=t_res->ai_next)
> +  /* Check that getaddrinfo() returned the used IP (FCrDNS technique). */
> +
> +  DBUG_PRINT("info", ("The following IP addresses found for '%s':",
> +                      (const char *) hostname_buffer));
> +
> +  for (struct addrinfo *addr_info= addr_info_list;
> +       addr_info; addr_info=addr_info->ai_next)
> +
>     {
> -    if (!memcmp(&(t_res->ai_addr), in,
> -                sizeof(struct sockaddr_storage) ) )
> +    struct sockaddr *resolved_ip= addr_info->ai_addr;
> +    char resolved_ip_key[HOST_ENTRY_KEY_SIZE];
> +
> +    if (hostname_cache_get_key(resolved_ip, resolved_ip_key))

Can't we still compare the addr structures?

>       {
> -      add_hostname(in, name);
> -      freeaddrinfo(res_lst);
> -      DBUG_RETURN(name);
> +      DBUG_PRINT("error", ("Can not get key for IP address."));
> +
> +      freeaddrinfo(addr_info_list);
> +      DBUG_RETURN(TRUE);
>       }
> +
> +    DBUG_PRINT("info", ("  - '%s'", (const char *) resolved_ip_key));
> +
> +    if (strcmp(ip_string, resolved_ip_key) == 0)
> +    {
> +      /* Copy host name string to be stored in the cache. */
> +
> +      *hostname= my_strdup(hostname_buffer, MYF(0));
> +
> +      if (!*hostname)
> +      {
> +        DBUG_PRINT("error", ("Out of memory."));
> +
> +        freeaddrinfo(addr_info_list);
> +        DBUG_RETURN(TRUE);
> +      }
> +
> +      break;
> +    }
> +  }
> +
> +  /* Log resolved IP-addresses if no match was found. */
> +
> +  if (!*hostname)
> +  {
> +    sql_print_information("Hostname '%s' does not resolve to '%s'.",
> +                          (const char *) hostname_buffer,
> +                          (const char *) ip_string);
> +    sql_print_information("Hostname '%s' has the following IP addresses:",
> +                          (const char *) hostname_buffer);
> +
> +    for (struct addrinfo *addr_info= addr_info_list;
> +         addr_info; addr_info=addr_info->ai_next)

IMHO, not needed. Admin can use dns tools and check.

> +    {
> +      struct sockaddr *resolved_ip= addr_info->ai_addr;
> +      char resolved_ip_key[HOST_ENTRY_KEY_SIZE];
> +
> +      if (hostname_cache_get_key(resolved_ip, resolved_ip_key))
> +        strncpy(resolved_ip_key, "n/a", 3);
> +
> +      sql_print_information(" - %s\n", (const char *) resolved_ip_key);
> +    }
> +  }
> +
> +  /* Free the result of getaddrinfo(). */
> +
> +  freeaddrinfo(addr_info_list);
> +
> +  /* Add an entry for the IP to the cache. */
> +
> +  if (*hostname)
> +  {
> +    add_hostname(ip_string, *hostname);
> +    *connect_errors= 0;
> +  }
> +  else
> +  {
> +    DBUG_PRINT("error",("Couldn't verify hostname with getaddrinfo()."));
> +
> +    add_unresolved_hostname(ip_string);
> +    *hostname= NULL;
> +    *connect_errors= 0;
>     }
> -  freeaddrinfo(res_lst);
> -  DBUG_PRINT("error",("Couldn't verify hostname with getaddrinfo"));
>
> -add_wrong_ip_and_return:
> -  my_free(name,MYF(0));
> -  add_wrong_ip(in);
> -  DBUG_RETURN(0);
> +  DBUG_RETURN(FALSE);
>   }
>

Looks good so far, thanks for working on this!

Regards,
Thread
bzr commit into mysql-5.4 branch (alik:2804) Bug#45584Alexander Nozdrin24 Jun
  • Re: bzr commit into mysql-5.4 branch (alik:2804) Bug#45584Davi Arnaut26 Jun