Re: [Maria-developers] [Commits] cae1945: MDEV-9117: Client Server capability negotiation for MariaDB specific functionality
Hi, Sanja! On Nov 25, OleksandrByelkin wrote:
revision-id: cae19452eec1d3be47fa04759dd2dda6d061543e (mariadb-10.1.8-69-gcae1945) parent(s): 55e67c3e344317c6f349f5391e5d117ec51ae062 committer: Oleksandr Byelkin timestamp: 2015-11-25 18:09:39 +0100 message:
MDEV-9117: Client Server capability negotiation for MariaDB specific functionality
New capability flags space. Removed old progress flag, added new one.
diff --git a/include/mysql_com.h b/include/mysql_com.h index aa6ab0f..57092dc 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -215,7 +216,8 @@ enum enum_server_command /* Don't close the connection for a connection with expired password. */ #define CLIENT_CAN_HANDLE_EXPIRED_PASSWORDS (1UL << 22)
-#define CLIENT_PROGRESS (1UL << 29) /* Client support progress indicator */ +#define CLIENT_PROGRESS_OBSOLETE (1UL << 29)
ok
+#define CLIENT_PROGRESS 0
why is that?
#define CLIENT_SSL_VERIFY_SERVER_CERT (1UL << 30) /* It used to be that if mysql_real_connect() failed, it would delete any @@ -228,6 +230,13 @@ enum enum_server_command */ #define CLIENT_REMEMBER_OPTIONS (1UL << 31)
+/* MariaDB extended capability flags */ +#define MARIADB_CLIENT_FLAGS_MASK 0xffffffff00000000ULL +/* Client support progress indicator */ +#define MARIADB_CLIENT_PROGRESS (1ULL << 32) +/* Client support mariaDB extended flags */ +#define MARIADB_CLIENT_EXTENDED_FLAGS (1ULL << 63)
how can this work, if client_flag and server_capabilities are only 32-bit variables?
+ #ifdef HAVE_COMPRESS #define CAN_CLIENT_COMPRESS CLIENT_COMPRESS #else diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 1240d5d..d07e0a4 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -11729,18 +11731,38 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, */ DBUG_ASSERT(net->read_pos[pkt_len] == 0);
- ulong client_capabilities= uint2korr(net->read_pos); + ulonglong client_capabilities= uint2korr(net->read_pos); + compile_time_assert(sizeof(client_capabilities) >= 8); if (client_capabilities & CLIENT_PROTOCOL_41) { - if (pkt_len < 4) + if (pkt_len < 32) return packet_error; client_capabilities|= ((ulong) uint2korr(net->read_pos+2)) << 16; + if (!(client_capabilities & CLIENT_MYSQL)) + { + // it is client with mariadb extensions + client_capabilities|= CLIENT_MYSQL; + ulonglong ext_client_capabilities= + (((ulonglong)uint4korr(net->read_pos + 28)) << 32); + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) + client_capabilities|= ext_client_capabilities; + else + { + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " + "flags: %llx ext flags %llx", + client_capabilities, ext_client_capabilities)); + return packet_error;
Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)?
+ } + }
here you need to check for old MariaDB clients - they will have CLIENT_MYSQL and might have CLIENT_PROGRESS_OBSOLETE - do something like else if (client_capabilities & CLIENT_PROGRESS_OBSOLETE) { client_capabilities |= MARIADB_CLIENT_PROGRESS; }
}
diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index a1a52e8..31fca0f 100644 --- a/tests/mysql_client_test.c +++ b/tests/mysql_client_test.c @@ -15336,7 +15336,7 @@ static void test_mysql_insert_id()
myheader("test_mysql_insert_id");
- rc= mysql_query(mysql, "drop table if exists t1"); + rc= mysql_query(mysql, "drop table if exists t1,t2");
separate commit, please
myquery(rc); /* table without auto_increment column */ rc= mysql_query(mysql, "create table t1 (f1 int, f2 varchar(255), key(f1))"); @@ -18700,7 +18700,8 @@ static void test_progress_reporting() myheader("test_progress_reporting");
conn= client_connect(CLIENT_PROGRESS, MYSQL_PROTOCOL_TCP, 0); - DIE_UNLESS(conn->client_flag & CLIENT_PROGRESS); + if (!(conn->client_flag & CLIENT_PROGRESS)) + return;
what are you testing here? Regards, Sergei Chief Architect MariaDB and security@mariadb.org -- Vote for my Percona Live 2016 talks: https://www.percona.com/live/data-performance-conference-2016/sessions/maria... https://www.percona.com/live/data-performance-conference-2016/sessions/maria...
On 16.01.2016 21:56, Sergei Golubchik wrote:
Hi, Sanja!
On Nov 25, OleksandrByelkin wrote:
revision-id: cae19452eec1d3be47fa04759dd2dda6d061543e (mariadb-10.1.8-69-gcae1945) parent(s): 55e67c3e344317c6f349f5391e5d117ec51ae062 committer: Oleksandr Byelkin timestamp: 2015-11-25 18:09:39 +0100 message:
MDEV-9117: Client Server capability negotiation for MariaDB specific functionality
New capability flags space. Removed old progress flag, added new one.
diff --git a/include/mysql_com.h b/include/mysql_com.h index aa6ab0f..57092dc 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -215,7 +216,8 @@ enum enum_server_command /* Don't close the connection for a connection with expired password. */ #define CLIENT_CAN_HANDLE_EXPIRED_PASSWORDS (1UL << 22)
-#define CLIENT_PROGRESS (1UL << 29) /* Client support progress indicator */ +#define CLIENT_PROGRESS_OBSOLETE (1UL << 29) ok
+#define CLIENT_PROGRESS 0 why is that? Because old flag will not work with the new server.
#define CLIENT_SSL_VERIFY_SERVER_CERT (1UL << 30) /* It used to be that if mysql_real_connect() failed, it would delete any @@ -228,6 +230,13 @@ enum enum_server_command */ #define CLIENT_REMEMBER_OPTIONS (1UL << 31)
+/* MariaDB extended capability flags */ +#define MARIADB_CLIENT_FLAGS_MASK 0xffffffff00000000ULL +/* Client support progress indicator */ +#define MARIADB_CLIENT_PROGRESS (1ULL << 32) +/* Client support mariaDB extended flags */ +#define MARIADB_CLIENT_EXTENDED_FLAGS (1ULL << 63) how can this work, if client_flag and server_capabilities are only 32-bit variables? it is not anymore (on the server) on non-connector client it will be lost but who cares if it doesnot support it ((I removed support by your request).
+ #ifdef HAVE_COMPRESS #define CAN_CLIENT_COMPRESS CLIENT_COMPRESS #else diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 1240d5d..d07e0a4 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -11729,18 +11731,38 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, */ DBUG_ASSERT(net->read_pos[pkt_len] == 0);
- ulong client_capabilities= uint2korr(net->read_pos); + ulonglong client_capabilities= uint2korr(net->read_pos); + compile_time_assert(sizeof(client_capabilities) >= 8); if (client_capabilities & CLIENT_PROTOCOL_41) { - if (pkt_len < 4) + if (pkt_len < 32) return packet_error; client_capabilities|= ((ulong) uint2korr(net->read_pos+2)) << 16; + if (!(client_capabilities & CLIENT_MYSQL)) + { + // it is client with mariadb extensions + client_capabilities|= CLIENT_MYSQL; + ulonglong ext_client_capabilities= + (((ulonglong)uint4korr(net->read_pos + 28)) << 32); + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) + client_capabilities|= ext_client_capabilities; + else + { + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " + "flags: %llx ext flags %llx", + client_capabilities, ext_client_capabilities)); + return packet_error; Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)? Server read or do not read more flags depend on it.
+ } + } here you need to check for old MariaDB clients - they will have CLIENT_MYSQL and might have CLIENT_PROGRESS_OBSOLETE - do something like
else if (client_capabilities & CLIENT_PROGRESS_OBSOLETE) { client_capabilities |= MARIADB_CLIENT_PROGRESS; }
}
diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index a1a52e8..31fca0f 100644 --- a/tests/mysql_client_test.c +++ b/tests/mysql_client_test.c @@ -15336,7 +15336,7 @@ static void test_mysql_insert_id()
myheader("test_mysql_insert_id");
- rc= mysql_query(mysql, "drop table if exists t1"); + rc= mysql_query(mysql, "drop table if exists t1,t2");
separate commit, please ok, but it looks too late now, sorry.
myquery(rc); /* table without auto_increment column */ rc= mysql_query(mysql, "create table t1 (f1 int, f2 varchar(255), key(f1))"); @@ -18700,7 +18700,8 @@ static void test_progress_reporting() myheader("test_progress_reporting");
conn= client_connect(CLIENT_PROGRESS, MYSQL_PROTOCOL_TCP, 0); - DIE_UNLESS(conn->client_flag & CLIENT_PROGRESS); + if (!(conn->client_flag & CLIENT_PROGRESS)) + return;
what are you testing here? yes, I'll fix it.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Oleksandr! On Jan 17, Oleksandr Byelkin wrote:
On 16.01.2016 21:56, Sergei Golubchik wrote:
On Nov 25, OleksandrByelkin wrote:
diff --git a/include/mysql_com.h b/include/mysql_com.h index aa6ab0f..57092dc 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -215,7 +216,8 @@ enum enum_server_command /* Don't close the connection for a connection with expired password. */ #define CLIENT_CAN_HANDLE_EXPIRED_PASSWORDS (1UL << 22)
-#define CLIENT_PROGRESS (1UL << 29) /* Client support progress indicator */ +#define CLIENT_PROGRESS_OBSOLETE (1UL << 29)
ok
+#define CLIENT_PROGRESS 0 why is that? Because old flag will not work with the new server.
Right, but for binaries (already compiled libraries and clients), the flag is (1UL << 29) - I mean, the number, not the "CLIENT_PROGRESS" name. And I suggest it to continue to work, as I wrote in the review, with something like if (client_capabilities & CLIENT_PROGRESS_OBSOLETE) client_capabilities|= MARIADB_CLIENT_PROGRESS; As for the *name* CLIENT_PROGRESS - it only affects clients/libraries that are compiled with the new MariaDB. And I'd rather see a compilation failure in this case ("unknown symbol CLIENT_PROGRESS") than a silently disabled feature. So, I think, you don't need to keep old CLIENT_PROGRESS symbol, you can safely remove it.
+/* Client support mariaDB extended flags */ +#define MARIADB_CLIENT_EXTENDED_FLAGS (1ULL << 63) how can this work, if client_flag and server_capabilities are only 32-bit variables? it is not anymore (on the server) on non-connector client it will be lost but who cares if it doesnot support it ((I removed support by your request).
Oh, yes, right. This is only the server part of the patch. Sorry.
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 1240d5d..d07e0a4 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -11729,18 +11731,38 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio,
+ if (!(client_capabilities & CLIENT_MYSQL)) + { + // it is client with mariadb extensions + client_capabilities|= CLIENT_MYSQL;
Wait, why is this?
+ ulonglong ext_client_capabilities= + (((ulonglong)uint4korr(net->read_pos + 28)) << 32); + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) + client_capabilities|= ext_client_capabilities; + else + { + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " + "flags: %llx ext flags %llx", + client_capabilities, ext_client_capabilities)); + return packet_error; Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)? Server read or do not read more flags depend on it.
But !(client_capabilities & CLIENT_MYSQL) already tells you that. It's a MariaDB client = it has extended flags.
diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index a1a52e8..31fca0f 100644 --- a/tests/mysql_client_test.c +++ b/tests/mysql_client_test.c @@ -15336,7 +15336,7 @@ static void test_mysql_insert_id()
myheader("test_mysql_insert_id");
- rc= mysql_query(mysql, "drop table if exists t1"); + rc= mysql_query(mysql, "drop table if exists t1,t2"); separate commit, please ok, but it looks too late now, sorry.
If you haven't pushed to 10.2 yet, it's not late. I'll ping you on irc and explain how to do it.
myquery(rc); /* table without auto_increment column */ rc= mysql_query(mysql, "create table t1 (f1 int, f2 varchar(255), key(f1))"); @@ -18700,7 +18700,8 @@ static void test_progress_reporting() myheader("test_progress_reporting");
conn= client_connect(CLIENT_PROGRESS, MYSQL_PROTOCOL_TCP, 0); - DIE_UNLESS(conn->client_flag & CLIENT_PROGRESS); + if (!(conn->client_flag & CLIENT_PROGRESS)) + return;
what are you testing here? yes, I'll fix it.
I was only asking a question :) I don't know what "it" is and what's wrong with it. Regards, Sergei Chief Architect MariaDB and security@mariadb.org -- Vote for my Percona Live 2016 talks: https://www.percona.com/live/data-performance-conference-2016/sessions/maria... https://www.percona.com/live/data-performance-conference-2016/sessions/maria...
On 17.01.2016 11:14, Sergei Golubchik wrote:
Hi, Oleksandr!
On Jan 17, Oleksandr Byelkin wrote:
On 16.01.2016 21:56, Sergei Golubchik wrote:
On Nov 25, OleksandrByelkin wrote:
diff --git a/include/mysql_com.h b/include/mysql_com.h index aa6ab0f..57092dc 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -215,7 +216,8 @@ enum enum_server_command /* Don't close the connection for a connection with expired password. */ #define CLIENT_CAN_HANDLE_EXPIRED_PASSWORDS (1UL << 22)
-#define CLIENT_PROGRESS (1UL << 29) /* Client support progress indicator */ +#define CLIENT_PROGRESS_OBSOLETE (1UL << 29) ok
+#define CLIENT_PROGRESS 0 why is that? Because old flag will not work with the new server. Right, but for binaries (already compiled libraries and clients), the flag is (1UL << 29) - I mean, the number, not the "CLIENT_PROGRESS" name. And I suggest it to continue to work, as I wrote in the review, with something like
if (client_capabilities & CLIENT_PROGRESS_OBSOLETE) client_capabilities|= MARIADB_CLIENT_PROGRESS;
As for the *name* CLIENT_PROGRESS - it only affects clients/libraries that are compiled with the new MariaDB. And I'd rather see a compilation failure in this case ("unknown symbol CLIENT_PROGRESS") than a silently disabled feature.
So, I think, you don't need to keep old CLIENT_PROGRESS symbol, you can safely remove it. OK (but check last comment please).
+/* Client support mariaDB extended flags */ +#define MARIADB_CLIENT_EXTENDED_FLAGS (1ULL << 63) how can this work, if client_flag and server_capabilities are only 32-bit variables? it is not anymore (on the server) on non-connector client it will be lost but who cares if it doesnot support it ((I removed support by your request). Oh, yes, right. This is only the server part of the patch. Sorry.
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 1240d5d..d07e0a4 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -11729,18 +11731,38 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, + if (!(client_capabilities & CLIENT_MYSQL)) + { + // it is client with mariadb extensions + client_capabilities|= CLIENT_MYSQL; Wait, why is this? Jast to return it "as it was" if you think that it is not needed then I remove it.
+ ulonglong ext_client_capabilities= + (((ulonglong)uint4korr(net->read_pos + 28)) << 32); + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) + client_capabilities|= ext_client_capabilities; + else + { + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " + "flags: %llx ext flags %llx", + client_capabilities, ext_client_capabilities)); + return packet_error; Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)? Server read or do not read more flags depend on it. But !(client_capabilities & CLIENT_MYSQL) already tells you that. It's a MariaDB client = it has extended flags. As you saw it was not.
diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c index a1a52e8..31fca0f 100644 --- a/tests/mysql_client_test.c +++ b/tests/mysql_client_test.c @@ -15336,7 +15336,7 @@ static void test_mysql_insert_id()
myheader("test_mysql_insert_id");
- rc= mysql_query(mysql, "drop table if exists t1"); + rc= mysql_query(mysql, "drop table if exists t1,t2"); separate commit, please ok, but it looks too late now, sorry. If you haven't pushed to 10.2 yet, it's not late. I'll ping you on irc and explain how to do it. I'll check what is pushed and what is not.
myquery(rc); /* table without auto_increment column */ rc= mysql_query(mysql, "create table t1 (f1 int, f2 varchar(255), key(f1))"); @@ -18700,7 +18700,8 @@ static void test_progress_reporting() myheader("test_progress_reporting");
conn= client_connect(CLIENT_PROGRESS, MYSQL_PROTOCOL_TCP, 0); - DIE_UNLESS(conn->client_flag & CLIENT_PROGRESS); + if (!(conn->client_flag & CLIENT_PROGRESS)) + return;
what are you testing here? yes, I'll fix it. I was only asking a question :) I don't know what "it" is and what's wrong with it. wrong (as I thought before) was that CLIENT_PROGRESS is defined as 0 now.
OK, maybe after some thinking: it is why I made CLIENT_PROGRESS defined as 0 old clients thinks that server just not support progress if it is not rewritten for new flags support.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Oleksandr! On Jan 17, Oleksandr Byelkin wrote:
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 1240d5d..d07e0a4 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -11729,18 +11731,38 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, + if (!(client_capabilities & CLIENT_MYSQL)) + { + // it is client with mariadb extensions + client_capabilities|= CLIENT_MYSQL; Wait, why is this? Jast to return it "as it was" if you think that it is not needed then I remove it.
What do you mean "return"? To whom? I don't see why you may need to pretend that this is MySQL client, when it is not.
+ ulonglong ext_client_capabilities= + (((ulonglong)uint4korr(net->read_pos + 28)) << 32); + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) + client_capabilities|= ext_client_capabilities; + else + { + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " + "flags: %llx ext flags %llx", + client_capabilities, ext_client_capabilities)); + return packet_error; Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)? Server read or do not read more flags depend on it. But !(client_capabilities & CLIENT_MYSQL) already tells you that. It's a MariaDB client = it has extended flags. As you saw it was not.
No, I did't. What do you mean "it was not"? Regards, Sergei Chief Architect MariaDB and security@mariadb.org -- Vote for my Percona Live 2016 talks: https://www.percona.com/live/data-performance-conference-2016/sessions/maria... https://www.percona.com/live/data-performance-conference-2016/sessions/maria...
On 17.01.2016 20:09, Sergei Golubchik wrote:
Hi, Oleksandr!
On Jan 17, Oleksandr Byelkin wrote:
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 1240d5d..d07e0a4 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -11729,18 +11731,38 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio, + if (!(client_capabilities & CLIENT_MYSQL)) + { + // it is client with mariadb extensions + client_capabilities|= CLIENT_MYSQL; Wait, why is this? Jast to return it "as it was" if you think that it is not needed then I remove it. What do you mean "return"? To whom? I don't see why you may need to pretend that this is MySQL client, when it is not. OK. (I set it back when it has other name but yes now it has no sens) + ulonglong ext_client_capabilities= + (((ulonglong)uint4korr(net->read_pos + 28)) << 32); + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) + client_capabilities|= ext_client_capabilities; + else + { + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " + "flags: %llx ext flags %llx", + client_capabilities, ext_client_capabilities)); + return packet_error; Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)? Server read or do not read more flags depend on it. But !(client_capabilities & CLIENT_MYSQL) already tells you that. It's a MariaDB client = it has extended flags. As you saw it was not. No, I did't. What do you mean "it was not"?
I meant flag CLIENT_MYSQL. But there is other function, report extended functionality to client (clein also can understand that it is mariaDB server, but the flag alwais set in extended flag area is additional check.
Hi, Oleksandr! On Jan 17, Oleksandr Byelkin wrote:
> + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) > + client_capabilities|= ext_client_capabilities; > + else > + { > + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " > + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " > + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " > + "flags: %llx ext flags %llx", > + client_capabilities, ext_client_capabilities)); > + return packet_error; Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)?
I meant flag CLIENT_MYSQL. But there is other function, report extended functionality to client (clein also can understand that it is mariaDB server, but the flag alwais set in extended flag area is additional check.
I don't understand. What additional safety does this additional check give you? Regards, Sergei Chief Architect MariaDB and security@mariadb.org -- Vote for my Percona Live 2016 talks: https://www.percona.com/live/data-performance-conference-2016/sessions/maria... https://www.percona.com/live/data-performance-conference-2016/sessions/maria...
On 17.01.2016 21:55, Sergei Golubchik wrote:
Hi, Oleksandr!
On Jan 17, Oleksandr Byelkin wrote:
>> + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) >> + client_capabilities|= ext_client_capabilities; >> + else >> + { >> + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " >> + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " >> + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " >> + "flags: %llx ext flags %llx", >> + client_capabilities, ext_client_capabilities)); >> + return packet_error; > Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)? I meant flag CLIENT_MYSQL. But there is other function, report extended functionality to client (clein also can understand that it is mariaDB server, but the flag alwais set in extended flag area is additional check. I don't understand. What additional safety does this additional check give you?
We checked the bytes of extended flag and the bit should be ON alway if it is MariaDB. It is not so good as some signature but better then nothing (especially taking into account that all bits are 0).
Hi, Oleksandr! On Jan 18, Oleksandr Byelkin wrote:
On 17.01.2016 21:55, Sergei Golubchik wrote:
On Jan 17, Oleksandr Byelkin wrote:
>>> + if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS) >>> + client_capabilities|= ext_client_capabilities; >>> + else >>> + { >>> + DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, " >>> + "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, " >>> + "but MARIADB_CLIENT_EXTENDED_FLAGS is off. " >>> + "flags: %llx ext flags %llx", >>> + client_capabilities, ext_client_capabilities)); >>> + return packet_error; >> Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)? I meant flag CLIENT_MYSQL. But there is other function, report extended functionality to client (clein also can understand that it is mariaDB server, but the flag alwais set in extended flag area is additional check. I don't understand. What additional safety does this additional check give you?
We checked the bytes of extended flag and the bit should be ON alway if it is MariaDB.
It is not so good as some signature but better then nothing (especially taking into account that all bits are 0).
I don't see how that helps. Normally these bytes are zero-filled in old clients. So all bits are naturally 0 and you can simply check for capabilities, like if (ext_client_capabilities & MARIADB_PROGRESS_REPORT) because it will just work and deliver correct result. So checking MARIADB_CLIENT_EXTENDED_FLAGS is unnecessary. If some broken third-party connector does not zero-fill these bytes, than your MARIADB_CLIENT_EXTENDED_FLAGS can be set and your check will simply produce wrong results. So, either way MARIADB_CLIENT_EXTENDED_FLAGS flag is not needed - it is sufficient to check for MYSQL_CLIENT flag. Regards, Sergei Chief Architect MariaDB and security@mariadb.org -- Vote for my Percona Live 2016 talks: https://www.percona.com/live/data-performance-conference-2016/sessions/maria... https://www.percona.com/live/data-performance-conference-2016/sessions/maria...
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik