Hi Andrey,
andrey@stripped wrote:
[..]
> ChangeSet@stripped, 2008-02-26 13:54:26+01:00, andrey@stripped +2 -0
> Fix for Bug #27944 Filtering THD::client capabilities
>
> Add all CLIENT_ flags to a define and use ti to filter the client_capabilities.
Please describe the potential problem and the solution.
> include/mysql_com.h@stripped, 2008-02-26 13:54:22+01:00, andrey@stripped
> +25 -0
> List all CLIENT flags in a common defition. Add also a definition
> which excludes flags, which are optoinal.
>
> sql/sql_connect.cc@stripped, 2008-02-26 13:54:22+01:00, andrey@stripped
> +16 -7
> Renamed client_flags to server_flags to reflect what the server supports.
> Only allow from the client the flags the server supports.
>
> diff -Nrup a/include/mysql_com.h b/include/mysql_com.h
> --- a/include/mysql_com.h 2007-12-14 14:01:09 +01:00
> +++ b/include/mysql_com.h 2008-02-26 13:54:22 +01:00
> @@ -148,6 +148,31 @@ enum enum_server_command
> #define CLIENT_SSL_VERIFY_SERVER_CERT (1UL << 30)
> #define CLIENT_REMEMBER_OPTIONS (1UL << 31)
>
> +#define CLIENT_ALL_FLAGS (CLIENT_LONG_PASSWORD | \
> + CLIENT_FOUND_ROWS | \
> + CLIENT_LONG_FLAG | \
> + CLIENT_CONNECT_WITH_DB | \
> + CLIENT_NO_SCHEMA | \
> + CLIENT_COMPRESS | \
> + CLIENT_ODBC | \
> + CLIENT_LOCAL_FILES | \
> + CLIENT_IGNORE_SPACE | \
> + CLIENT_PROTOCOL_41 | \
> + CLIENT_INTERACTIVE | \
> + CLIENT_SSL | \
> + CLIENT_IGNORE_SIGPIPE | \
> + CLIENT_TRANSACTIONS | \
> + CLIENT_RESERVED | \
> + CLIENT_SECURE_CONNECTION | \
> + CLIENT_MULTI_STATEMENTS | \
> + CLIENT_MULTI_RESULTS | \
> + CLIENT_SSL_VERIFY_SERVER_CERT | \
> + CLIENT_REMEMBER_OPTIONS)
Please add a comment explaining what is this define for and what should
be in it and what should not.
> +#define CLIENT_BASIC_FLAGS (((CLIENT_ALL_FLAGS & ~CLIENT_SSL) \
> + & ~CLIENT_COMPRESS) \
> + &
> ~CLIENT_SSL_VERIFY_SERVER_CERT)
> +
Please add a comment explaining.
> #define SERVER_STATUS_IN_TRANS 1 /* Transaction has started */
> #define SERVER_STATUS_AUTOCOMMIT 2 /* Server in auto_commit mode */
> #define SERVER_MORE_RESULTS_EXISTS 8 /* Multi query - next query exists */
> diff -Nrup a/sql/sql_connect.cc b/sql/sql_connect.cc
> --- a/sql/sql_connect.cc 2008-02-19 13:45:16 +01:00
> +++ b/sql/sql_connect.cc 2008-02-26 13:54:22 +01:00
> @@ -715,20 +715,24 @@ static int check_connection(THD *thd)
> bzero((char*) &thd->remote, sizeof(thd->remote));
> }
> vio_keepalive(net->vio, TRUE);
> +
> + ulong server_flags;
I think a better name would be server_capabilities.
> {
> /* buff[] needs to big enough to hold the server_version variable */
> char buff[SERVER_VERSION_LENGTH + SCRAMBLE_LENGTH + 64];
> - ulong client_flags = (CLIENT_LONG_FLAG | CLIENT_CONNECT_WITH_DB |
> - CLIENT_PROTOCOL_41 | CLIENT_SECURE_CONNECTION);
> + server_flags= CLIENT_BASIC_FLAGS;
>
> if (opt_using_transactions)
> - client_flags|=CLIENT_TRANSACTIONS;
> + server_flags|= CLIENT_TRANSACTIONS;
> #ifdef HAVE_COMPRESS
> - client_flags |= CLIENT_COMPRESS;
> + server_flags|= CLIENT_COMPRESS;
> #endif /* HAVE_COMPRESS */
> #ifdef HAVE_OPENSSL
> if (ssl_acceptor_fd)
> - client_flags |= CLIENT_SSL; /* Wow, SSL is available! */
> + {
> + server_flags |= CLIENT_SSL; /* Wow, SSL is available! */
> + server_flags |= CLIENT_SSL_VERIFY_SERVER_CERT;
> + }
> #endif /* HAVE_OPENSSL */
>
> end= strnmov(buff, server_version, SERVER_VERSION_LENGTH) + 1;
> @@ -747,7 +751,7 @@ static int check_connection(THD *thd)
> */
> end= strmake(end, thd->scramble, SCRAMBLE_LENGTH_323) + 1;
>
> - int2store(end, client_flags);
> + int2store(end, server_flags);
> /* write server characteristics: up to 16 bytes allowed */
> end[2]=(char) default_charset_info->number;
> int2store(end+3, thd->server_status);
> @@ -777,7 +781,7 @@ static int check_connection(THD *thd)
> if (thd->packet.alloc(thd->variables.net_buffer_length))
> return 1; /* The error is set by alloc(). */
>
> - thd->client_capabilities=uint2korr(net->read_pos);
> + thd->client_capabilities= uint2korr(net->read_pos);
> if (thd->client_capabilities & CLIENT_PROTOCOL_41)
> {
> thd->client_capabilities|= ((ulong) uint2korr(net->read_pos+2)) <<
> 16;
> @@ -792,6 +796,11 @@ static int check_connection(THD *thd)
> thd->max_client_packet_length= uint3korr(net->read_pos+2);
> end= (char*) net->read_pos+5;
> }
> + /*
> + Disable those bits which are not supported by the server.
> + This is a precautionary measure, if the client lies. See Bug#27944.
> + */
> + thd->client_capabilities&= server_flags;
Shouldn't a error be thrown if the client sends a flag which is not
supported by the server? This also needs a test case.
Regards,
--
Davi Arnaut, Software Engineer
MySQL Inc, www.mysql.com
Are you MySQL certified? www.mysql.com/certification
