fix detection of client version on MariaDB 10.6.x

[bugfood]

For mysql_get_client_version(), MariaDB 10.6.2+ reports the version of the mariadb-connector-c library rather than the server version: https://jira.mariadb.org/browse/CONC-509

The mariadb-connector-c library versioning does not track the versioning of the main MariaDB client/server suite, so the change broke the client version comparisons here in DBD-mysql. The previous behavior of reporting the server version probably mostly worked by accident, since server and client versions tend to be roughly aligned in user environments.

To fix this, change DBD-mysql to use a function which is documented to return the appropriate client version information. https://mariadb.com/kb/en/mariadb_get_infov/

This fixes: https://github.com/perl5-dbi/DBD-mysql/issues/333 ...and is a minor revision of @bviviano's original fix therein.

[bugfood]

I can see that this will need some more work to allow for MySQL and for MariaDB before this commit:

https://github.com/mariadb-corporation/mariadb-connector-c/commit/d73e4c23a26feafbb7c4c5171b809506822322ce

I don't know how to handle that, though. my first guess was to use #ifdef MARIADB_CLIENT_VERSION_ID, but it turns out that MARIADB_CLIENT_VERSION_ID is an enum value, not a define, so the preprocessor can't work with that.

This only way I know to do this is by a configure-time check (before compilation) which does a compilation of a test program like:

#include <mysql.h>

int main(void) {
    unsigned int version;
    mariadb_get_infov(NULL, MARIADB_CLIENT_VERSION_ID, &version);
    return 0;
}

...and then sets e.g. -DMARIA_GETGET_INFOV in the C flags.

I don't see how to do this using the perl-based Makefile generation. Can somebody give me a hint? I looked through this and I don't see how: https://perldoc.perl.org/ExtUtils::MakeMaker

I haven't seen any alternative approaches that would work either.

I'd be happy to revise this, but I'll need some guidance.

[bugfood]

I had a second look at this and it turns out to be easier than I thought. DBD-mysql already does not support such old mariadb-connector-c versions.

https://github.com/perl5-dbi/DBD-mysql/blob/master/.travis.yml#L144

So, all we have to do is conditionalize MariaDB vs. MySQL, which we already do via an #ifdef.

The updated PR works for me and passes the tests.

Please merge the PR or let me know if there is something I need to change.

Thanks, Corey

[toddr]

This fix is a priority for us because it makes it impossible to connect to MariaDB Server 5.5 (using the mariadb-connector v3.2.6) with SSL certificates unless we set mysql_ssl_optional=1.

Failing to set this leads to a somewhat confusing error message on connect:

SSL connection error: Enforcing SSL encryption is not supported

We would prefer not to have to set this.

In testing this patch, it causes a stack smash. The issue it turns out is that while the documentation says the variable is a unsigned int, the call is injecting the result into a size_t, which I assume is a different size at least on CentOS 7.

A hack-ish solution is to do this instead?

- mariadb_get_infov(NULL, MARIADB_CLIENT_VERSION_ID, &version);
+ version = MARIADB_VERSION_ID;

[pali]

Hello! Is this issue also with DBD::MariaDB module?

[toddr]

Hello! Is this issue also with DBD::MariaDB module?

I don't know. Probably unless they fixed it? We need a DBD that can connect to all MySQL derivatives. We use DBD::MySQL for that but we link to the mariadb-connector library.

[toddr]

actually a better fix would be to switch it to a size_t as the library uses.

[toddr]

FTR, https://mariadb.com/kb/en/mariadb_get_infov/ documents MARIADB_CLIENT_VERSION_ID as unsigned int however the underlying code does this:

  case MARIADB_CLIENT_VERSION_ID:
    *((size_t *)arg)= MARIADB_VERSION_ID;
    break;

[toddr]

Also FTR, bisect shows the upstream change that broke this as: https://github.com/mariadb-corporation/mariadb-connector-c/commit/a37b7c3965706f9a062baaba0c494dd6efb2c306

[toddr]

I've provided the alternate version of the patch which just switches the unsigned int to a size_t.

I have tested this against mariadb-connector v3.2.6 and it works like a champ!

https://github.com/perl5-dbi/DBD-mysql/pull/342

[pali]

I have looked at this pull request and similar fix is needed also for DBD::MariaDB. As this is issue is about MariaDB and not MySQL, I'm planning to look at it and fix it in DBD::MariaDB, ideally with the new release.

Anyway, commit / pull request description is wrong.

MariaDB 10.6.2+ reports the version of the mariadb-connector-c library rather than the server version The previous behavior of reporting the server version probably mostly worked by accident, since server and client versions tend to be roughly aligned in user environments.

Function mysql_get_client_version() in previous MariaDB clients reported client version and not server version as is incorrectly mentioned. MariaDB 10.6.2+ reports reports mariadb-connector-c library version, known as package version.

So second sentence about mostly worked by accident is also not correct. It worked correctly because mysql_get_client_version() reported client version correctly.

@toddr's bisected link just proves it, previously it reported compile time macro MARIADB_VERSION_ID which is client version (obviously it cannot be server as it does not check any connection with server) and after that change it reports compile time macro MARIADB_PACKAGE_VERSION_ID which is package version.

[toddr]

@pali I'm happy to update the commit any way you like it or you're welcome to put in your own commit. Let me know what you prefer.

[pali]

I'm still investigation what happened. But it is really pity that API of such critical function was changed in backward incompatible manner.

[toddr]

I'm still investigation what happened. But it is really pity that API of such critical function was changed in backward incompatible manner.

I couldn't agree more. The fact that even the one we're replacing it with is incorrectly documented makes me nervous they're going to break that too (again)

[bugfood]

If the code actually uses size_t, then sure, that would be appropriate. I can't test that out myself right at this moment. I do suggest putting in a comment about the discrepancy, though, or else somebody later on might be confused about why it differs from the size documented.

Function mysql_get_client_version() in previous MariaDB clients reported client version and not server version as is incorrectly mentioned. MariaDB 10.6.2+ reports reports mariadb-connector-c library version, known as package version.

I was going by the subject of: https://jira.mariadb.org/browse/CONC-509 "mysql_get_client_version should return C/C version, not server version"

The commit for it even says so: https://github.com/mariadb-corporation/mariadb-connector-c/commit/a37b7c3965706f9a062baaba0c494dd6efb2c306

Instead of server version the api functions mysql_get_client_info and
mysql_get_client_version should return MARIADB_PACKAGE_VERSION/ID.

...though I do agree that it looks more like the code reported the client version previously. If that really is the case, then I don't understand the purpose of that commit to mariadb-connector-c.

[pali]

Ok, so it is really how I have written it. So fell free to update info in commit message.

Possible exchange of client and server version in referenced ticket/commit could happen because in past client library was part of the server package and build process shared same version compile time macro for client and server version. So in past if you installed both client and server from the same source and used local client to connect to the local server then client and server had same version numbers. Later new client was written (named Connector/C), still used same versioning scheme internally (5.x.y) but package version string was introduced which referenced the new client version scheme.

So probably server version in referenced ticket/commit means the real client version in old versioning scheme and client version means the package version = client version in new versioning scheme.

And now this new client version scheme (package) started to be returned by the old client API function mysql_get_client_version().

makes me nervous they're going to break that too (again)

DBD::MariaDB supports lot of other combinations of clients, connectors and servers than DBD::mysql. And if you look into sources you can find lot of new version checks. And all is just because client library is not backward compatible.

So yes, I agree, we have to just take it as a fact, that it will be broken again.

Anyway, I prepared different fix for DBD::MariaDB which should be more robust and handles also older Connector/C versions without that mariadb_get_infov() function as DBD::MariaDB supports also older Connector/C versions.

[bugfood]

Thanks @toddr and @pali. I have updated this PR with your fixes.

[toddr]

@bugfood, I still see it set as an unsigned int? it needs to be size_t

[bugfood]

@bugfood, I still see it set as an unsigned int? it needs to be size_t

Oops; fixed now.

[wjhurley]

What's the status of this pull request? It looks like the last issue was addressed over 4 months ago.

[bugfood]

What's the status of this pull request? It looks like the last issue was addressed over 4 months ago.

I've been using it on my desktop and haven't had any issues.

If there's anything else I need to do in order to get this reviewed, I don't know what that would be.

[toddr]

This seems to be identical to the patch we've been using minus some comments:

diff --git a/modules/DBD-mysql/DBD-mysql/dbdimp.h b/modules/DBD-mysql/DBD-mysql/dbdimp.h
index c592704766..56bb3800c0 100644
--- a/modules/DBD-mysql/DBD-mysql/dbdimp.h
+++ b/modules/DBD-mysql/DBD-mysql/dbdimp.h
@@ -106,7 +106,10 @@
 /* MYSQL_OPT_SSL_VERIFY_SERVER_CERT automatically enforce SSL mode */
 static inline bool ssl_verify_also_enforce_ssl(void) {
 #ifdef MARIADB_BASE_VERSION
-       my_ulonglong version = mysql_get_client_version();
+    size_t version;
+    /* This will fail to compile with mariadb-connector-c < v3.0.2, but such
+       old versions are untested and unsupported anyway. */
+    mariadb_get_infov(NULL, MARIADB_CLIENT_VERSION_ID, &version);
        return ((version >= 50544 && version < 50600) || (version >= 100020 && version < 100100) || version >= 100106);
 #else
        return false;
@@ -115,10 +118,12 @@ static inline bool ssl_verify_also_enforce_ssl(void) {
 
 /* MYSQL_OPT_SSL_VERIFY_SERVER_CERT is not vulnerable (CVE-2016-2047) and can be used */
 static inline bool ssl_verify_usable(void) {
-       my_ulonglong version = mysql_get_client_version();
 #ifdef MARIADB_BASE_VERSION
+    size_t version;
+    mariadb_get_infov(NULL, MARIADB_CLIENT_VERSION_ID, &version);
        return ((version >= 50547 && version < 50600) || (version >= 100023 && version < 100100) || version >= 100110);
 #else
+    my_ulonglong version = mysql_get_client_version();
        return ((version >= 50549 && version < 50600) || (version >= 50630 && version < 50700) || version >= 50712);
 #endif
 }

[wjhurley]

This seems to be identical to the patch we've been using minus some comments:

diff --git a/modules/DBD-mysql/DBD-mysql/dbdimp.h b/modules/DBD-mysql/DBD-mysql/dbdimp.h
index c592704766..56bb3800c0 100644
--- a/modules/DBD-mysql/DBD-mysql/dbdimp.h
+++ b/modules/DBD-mysql/DBD-mysql/dbdimp.h
@@ -106,7 +106,10 @@
 /* MYSQL_OPT_SSL_VERIFY_SERVER_CERT automatically enforce SSL mode */
 static inline bool ssl_verify_also_enforce_ssl(void) {
 #ifdef MARIADB_BASE_VERSION
-       my_ulonglong version = mysql_get_client_version();
+    size_t version;
+    /* This will fail to compile with mariadb-connector-c < v3.0.2, but such
+       old versions are untested and unsupported anyway. */
+    mariadb_get_infov(NULL, MARIADB_CLIENT_VERSION_ID, &version);
        return ((version >= 50544 && version < 50600) || (version >= 100020 && version < 100100) || version >= 100106);
 #else
        return false;
@@ -115,10 +118,12 @@ static inline bool ssl_verify_also_enforce_ssl(void) {
 
 /* MYSQL_OPT_SSL_VERIFY_SERVER_CERT is not vulnerable (CVE-2016-2047) and can be used */
 static inline bool ssl_verify_usable(void) {
-       my_ulonglong version = mysql_get_client_version();
 #ifdef MARIADB_BASE_VERSION
+    size_t version;
+    mariadb_get_infov(NULL, MARIADB_CLIENT_VERSION_ID, &version);
        return ((version >= 50547 && version < 50600) || (version >= 100023 && version < 100100) || version >= 100110);
 #else
+    my_ulonglong version = mysql_get_client_version();
        return ((version >= 50549 && version < 50600) || (version >= 50630 && version < 50700) || version >= 50712);
 #endif
 }

Is this patch published yet?

[bugfood]

Is this patch published yet?

The content of that patch is effectively the same as this PR.

I think the history there is:

1. I made this PR.
2. @toddr pulled in my change, pointed out the error in unsigned int, and made a local change to size_t.
3. I fixed this PR to use size_t as well and added a comment to explain why size_t is correct.

[toddr]

@dveeden could we get feedback or a merge of this PR, please?

[cbrandtbuffalo]

Adding a +1 for this fix. We got hit by this issue today after patching updated some MariaDB libraries.

[toddr]

I've closed my PR. It was only an example. Happy to see #339 merged and published. We've been using the latest release with this patch on top of it in production on several hundred thousand servers without issue for 6+ months.

[toddr]

@pali do you have merge/release privs here or do we need to get @dveeden involved?

[dveeden]

In master compiling against MariaDB client libs is no longer supported. Use DBD::MariaDB for MariaDB.