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
#--------------------------------------------------------------------
