Fix GH-13452: Fixed handshake response [mysqlnd] #13470

SakiTakamachi · 2024-02-22T00:37:44Z

please see #13452

It's probably safe to use MariaDB if the MySQL test passes, but ideally it would be safer to have a MariaDB test as well.

By the way, this problem also occurs in MySQL when I make utf8mb4_de_pb_0900_ai_ci the default collation charset.

In conclusion, regarding the character set in question here, both utf8mb4_general_ci and utf8mb4_unicode_ci can be considered the same utf8mb4, since collation is not relevant.

Therefore, we can purely use ???_general_ci.

The remaining issue is compatibility with older versions. MySQL introduced utf8mb4 from 5.5. MariaDB5.5 is compatible with MySQL5.5. All later Maria versions are 10 or higher, so I decided to check whether the version value is 50500 or higher as a criterion for whether to use utf8mb4.

SakiTakamachi · 2024-02-22T00:58:33Z

Looks like I need to check about packet->charset_no as well.

mvorisek · 2024-02-22T09:18:25Z

ext/mysqlnd/mysqlnd_commands.c

@@ -642,15 +642,6 @@ MYSQLND_METHOD(mysqlnd_command, handshake)(MYSQLND_CONN_DATA * const conn, const
 	conn->protocol_version	= greet_packet.protocol_version;
 	conn->server_version	= mnd_pestrdup(greet_packet.server_version, conn->persistent);

-	conn->greet_charset = mysqlnd_find_charset_nr(greet_packet.charset_no);


MariaDB 11.3 Docker images have been patched - MariaDB/mariadb-docker@d7a950d

but the GH-13452 can be tested using 11.3.1-rc image - https://hub.docker.com/layers/library/mariadb/11.3.1-rc/images/sha256-f37b1ba4f15fb90679f8f85fd7300815ef4c5ff7ada668bccbdff33e2ad8ecd6

Note can also test by placing --character-set-collations=utf8mb4=uca1400_ai_ci as the command on the 11.3+ image to enforce the setting.

nielsdos · 2024-02-22T16:38:41Z

If the mysqlnd structs are part of the public API (which I think is the case), then we should not remove the field in stable branches to avoid breaking binary compatibility with 3rd party drivers.
We should keep the field and keep it set to NULL in that case, and only remove it in master.

SakiTakamachi · 2024-02-22T16:58:58Z

@nielsdos
Thanks, you are right. I forgot about the API

kamil-tekiela · 2024-02-22T19:25:04Z

I wonder what this actually does. From a quick look, it seems like we just resend the same value back to MySQL. Presumably, we should not be doing that. We should only be sending the charset number that the client set, which can probably happen with PDO, but not mysqli. Although, I am not sure how that works. Why do we even send the charset number? I'll look into it more, but it seems to me like we could just send harcoded 0 or the value from conn object if it's somehow set.

SakiTakamachi · 2024-02-23T00:52:52Z

@kamil-tekiela

This always happens on the first handshake of a connection, so it probably should happen on mysqli as well.

What I'm trying to understand now is how a single byte of charset that I send from the client to the database server works. If there is any impact, a BC Break will occur at the same time as the bug is fixed.

However, I have no idea how it would be handled correctly by sending only the lower 8 bits of values greater than 255.

SakiTakamachi · 2024-02-23T01:44:40Z

I tried this.

php-src/ext/mysqlnd/mysqlnd_wireprotocol.c

Line 395 in 171e398

packet->charset_no = uint1korr(p);

Change as follows:

packet->charset_no = 30;

php:

<?php

$pdo = new PDO(/* mysql */);

$pdo->exec('CREATE DATABASE CTEST');
$r = $pdo->query("SELECT DEFAULT_CHARACTER_SET_NAME, DEFAULT_COLLATION_NAME FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME = 'CTEST';");

var_dump($r->fetchAll(PDO::FETCH_ASSOC));

result:

array(1) {
  [0]=>
  array(2) {
    ["DEFAULT_CHARACTER_SET_NAME"]=>
    string(7) "utf8mb4"
    ["DEFAULT_COLLATION_NAME"]=>
    string(18) "utf8mb4_0900_ai_ci"
  }
}

The ID of utf8mb4_0900_ai_ci is 255. My environment is MySQL8.0, so this result means that the default value of charset has not changed anything.

kamil-tekiela · 2024-02-23T01:48:39Z

I don't think changing charset_no would have any impact on the charset of your tables. In fact, I believe it should NOT affect it in any way as that would be wrong.

The value in charset_no specifies which charset is used in transit between the client and the server. It's known as the connection charset. It's not the same as the table/db/column charset.

SakiTakamachi · 2024-02-23T01:56:24Z

At least the charset_no included in the handshake initially sent from the database server involves the charset of the collation.

Since the value of the lower 8 bits is sent, if a charset with an ID of 256 is specified as the collation order, the value sent will be 0 and an error will occur during mysqlnd validation.

e.g. /etc/mysql/my.cnf

[mysqld]
character-set-server=utf8mb4
collation-server=utf8mb4_de_pb_0900_ai_ci

This is an error because the ID of utf8mb4_de_pb_0900_ai_ci is 256.

SakiTakamachi · 2024-02-23T02:03:32Z

@kamil-tekiela
But your comment is very helpful. As before, I checked the connection character set with ID30 specified.

result:

array(8) {
  [0]=>
  array(2) {
    ["Variable_name"]=>
    string(20) "character_set_client"
    ["Value"]=>
    string(6) "latin5"
  }
  [1]=>
  array(2) {
    ["Variable_name"]=>
    string(24) "character_set_connection"
    ["Value"]=>
    string(6) "latin5"
  }
  [2]=>
  array(2) {
    ["Variable_name"]=>
    string(22) "character_set_database"
    ["Value"]=>
    string(7) "utf8mb4"
  }
  [3]=>
  array(2) {
    ["Variable_name"]=>
    string(24) "character_set_filesystem"
    ["Value"]=>
    string(6) "binary"
  }
  [4]=>
  array(2) {
    ["Variable_name"]=>
    string(21) "character_set_results"
    ["Value"]=>
    string(6) "latin5"
  }
  [5]=>
  array(2) {
    ["Variable_name"]=>
    string(20) "character_set_server"
    ["Value"]=>
    string(7) "utf8mb4"
  }
  [6]=>
  array(2) {
    ["Variable_name"]=>
    string(20) "character_set_system"
    ["Value"]=>
    string(7) "utf8mb3"
  }
  [7]=>
  array(2) {
    ["Variable_name"]=>
    string(18) "character_sets_dir"
    ["Value"]=>
    string(26) "/usr/share/mysql/charsets/"
  }
}

CLI result:

+--------------------------+----------------------------+
| Variable_name            | Value                      |
+--------------------------+----------------------------+
| character_set_client     | utf8mb4                    |
| character_set_connection | utf8mb4                    |
| character_set_database   | utf8mb4                    |
| character_set_filesystem | binary                     |
| character_set_results    | utf8mb4                    |
| character_set_server     | utf8mb4                    |
| character_set_system     | utf8mb3                    |
| character_sets_dir       | /usr/share/mysql/charsets/ |
+--------------------------+----------------------------+

We can see that it has been changed to latin5. In other words, the charset_no sent by the server during the handshake and the charset_no that should be returned from the client in the handshake response have the same name but are completely different.

(edit)

default server a_protocol_character_set, only the lower 8-bits

https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_connection_phase_packets_protocol_handshake_v10.html

This is actually a collation character set.

SakiTakamachi · 2024-02-23T02:20:27Z

To fix this issue while avoiding BC Break:

In the stable branch, only if charset_no is 0, it will be reset to 255, which is the default value of MySQL8 series.
In the master branch, we stop reading the charset_no of handshake packets and standardize them all to 255.

How do you think?

SakiTakamachi · 2024-02-23T03:56:21Z

In the stable branch, only if charset_no is 0, it will be reset to 255, which is the default value of MySQL8 series.

First, I fixed it like that. It seems better to do the change to master as a follow-up in another PR.

SakiTakamachi · 2024-02-23T07:32:04Z

ext/mysqlnd/mysqlnd_commands.c

@@ -642,7 +643,7 @@ MYSQLND_METHOD(mysqlnd_command, handshake)(MYSQLND_CONN_DATA * const conn, const
 	conn->protocol_version	= greet_packet.protocol_version;
 	conn->server_version	= mnd_pestrdup(greet_packet.server_version, conn->persistent);

-	conn->greet_charset = mysqlnd_find_charset_nr(greet_packet.charset_no);
+	conn->greet_charset = mysqlnd_find_charset_nr(greet_packet.charset_no != 0 ? greet_packet.charset_no : MYSQLND_DEFAULT_PROTOCOL_CHARSET);


I left L647〜 as a fallback, but it looks like it could be UNEXPECTED.

kamil-tekiela · 2024-02-23T18:34:09Z

I see what you are trying to do here, Saki, and it makes sense, but I am worried we can't actually do that. We don't know if the server supports this charset.

The reason for this check was that we allow the user to select their preferred character set, but if they don't (in case of mysqli it's even impossible) then we need to provide the server with some default character set ID. What better way than resending the one that we received from the server in the handshake? But as we can see here, this value cannot be relied upon. MariaDB might add a gracious fallback for this issue, but we should get rid of this feature in mysqlnd anyway. So if we cannot use the value received from the server, how do we pick a default character set that will work?

The line you are changing is actually the fallback for when the character set wasn't chosen by the user. What you are suggesting is that we add a fallback to this fallback. We could probably cut the middle out completely. After all, this value has proven to be unreliable and could cause other bugs too. Let's not read the greet_packet.charset_no at all. However, passing some hardcoded ID is not a good idea either. I don't know what happens when the server receives an ID that is not supported, but if at best the server replies with an error then we leave our PHP users in a situation without a fix. We could use an ID that is more likely to be supported everywhere, e.g. 8 but we still run a risk of providing a value which could cause issues.

Unfortunately, I don't have a better idea as of now. Another issue is that many PHP developers have come to rely on this feature. It would actually be a pretty significant breaking change for recklessly designed projects.

kamil-tekiela · 2024-02-23T19:07:29Z

To minimize the impact of this issue we could change the code to only check the value of greet_packet.charset_no if the user hasn't already provided a valid charset. This would at least help PDO users.

SakiTakamachi · 2024-02-23T23:25:49Z

@kamil-tekiela

Thanks for the confirmation.

Overwrite it after this processing, probably with a user-specified charset. Here we are simply validating the value sent by the server.

I think the safest way to do this is to look up all the past MySQL and MariaDB default values and set the appropriate default values for each server version. Fortunately the version information is included in the handshake packet, so we might be able to do this.

However, we should only do this as a fallback in stable branches when charset_no is 0, as we risk breaking user environments that happen to rely on the current bad behavior.

It's true that we shouldn't read the charset of the handshake packet, so we should avoid doing so in master.

(edit)
I'm really curious how libmysql handles this...

SakiTakamachi · 2024-02-25T03:20:10Z

When connecting to a MySQL8.0 server with libmysql, the connection character set is utf8mb4. I got same result if I default the server collation to latin1. Presumably, libmysql automatically sets the initial value of the corresponding version if the connection character set is unspecified.

kamil-tekiela · 2024-02-25T20:04:02Z

Maybe we can do something similar. Can you check how they do it in their C code?

SakiTakamachi · 2024-02-26T08:59:22Z

libmysql is hard-coding default values as constants. This is possible because there is a driver for each version of MySQL...

However, if we could provide some default values for each version, it would be possible to do something similar.

SakiTakamachi · 2024-02-28T14:10:45Z

I thought about this for a while, and I think it's better to use utf8mb4_general_ci(45) for MySQL 5.5 or higher, and utf8mb3_general_ci(33) for older versions.

grooverdan · 2024-02-29T01:16:47Z

utf8mb4_general_ci

In 5.5 there got added utf8mb4_unicode_ci (224) which seems a better default base on SO.

SakiTakamachi · 2024-02-29T16:15:17Z

I consulted the MariaDB team on #13452

In conclusion, regarding the character set in question here, both utf8mb4_general_ci and utf8mb4_unicode_ci can be considered the same utf8mb4, since collation is not relevant.

Therefore, we can purely use ???_general_ci.

The remaining issue is compatibility with older versions. MySQL introduced utf8mb4 from 5.5. MariaDB5.5 is compatible with MySQL5.5. All later Maria versions are 10 or higher, so I decided to check whether the version value is 50500 or higher as a criterion for whether to use utf8mb4.

@kamil-tekiela How do you think that?

SakiTakamachi · 2024-02-29T16:23:51Z

ext/mysqlnd/mysqlnd_commands.c

-		mnd_sprintf_free(msg);
-		goto err;
+	const MYSQLND_CHARSET *read_charset = mysqlnd_find_charset_nr(greet_packet.charset_no);
+	if (!read_charset) {


If you look closely at mysqlnd_charset.c, there are some missing numbers in the character set ID. (e.g. 76, 100-127)

So, checking for 0 is not enough here. We need to check whether the corresponding character set exists.

Yes, Saki. That's why I feel that the way the current code works is correct. It raises an error whenever an unexpected charset is provided. 0 doesn't have any special meaning here.

What you are suggesting would probably be ok, but not ideal. utf8 and utf8mb4 should be a viable fallback value. The advantage is that we open the connection despite receiving an invalid charset value from the server. The disadvantage is that we silently switch to a different charset than a user might have expected. A properly designed PHP application will set the desired charset anyway.

So I would suggest using the two constant values as a fallback if we receive an unknown value from the server. Then we can think if we can drop this feature from mysqlnd in a major release. Maybe 9.0 would be good to do this.

Thank you for confirmation.

So I would suggest using the two constant values as a fallback if we receive an unknown value from the server.

If I didn't make a mistake, it should already work as a fallback like you mention. Two constants are used when mysqlnd_find_charset_nr() cannot find a character set. If it can find, uses the charset it finds in L651.

Then we can think if we can drop this feature from mysqlnd in a major release. Maybe 9.0 would be good to do this.

Agree. Initially I was thinking of using 8.4, but 9.x might be ideal.

kamil-tekiela

Yeah, the logic looks fine. I doubt we can write a test for this, though.

SakiTakamachi · 2024-02-29T17:14:57Z

Yeah, the logic looks fine. I doubt we can write a test for this, though.

We can create the same condition as this problem by rewriting my.cnf and restarting MySQL, but it can be an expensive test... I can't think of a realistic way to test this, so I might just commit as is for now.

The character set ID included in the handshake data at the time of connection actually only includes the lower 8 bits of the ID, so if try to use this to specify a character set, the corresponding character set may not exist. In case of an invalid character set, the default character set is now used without an error. Fixes #13452 Closes #13470

* PHP-8.2: Fixed handshake response charset. (#13470) [skip ci] Fixed NEWS

* PHP-8.3: NEWS Fixed handshake response charset. (#13470) Fixed handshake response charset. (#13470) [skip ci] Fixed NEWS

github-actions bot added the Extension: mysqlnd label Feb 22, 2024

SakiTakamachi changed the base branch from master to PHP-8.2 February 22, 2024 00:40

SakiTakamachi force-pushed the fix/gh-13452 branch from e771f74 to f92bb21 Compare February 22, 2024 00:40

SakiTakamachi marked this pull request as draft February 22, 2024 00:50

mvorisek reviewed Feb 22, 2024

View reviewed changes

SakiTakamachi marked this pull request as ready for review February 22, 2024 15:25

SakiTakamachi requested review from nielsdos and kamil-tekiela and removed request for kamil-tekiela and nielsdos February 22, 2024 15:25

SakiTakamachi marked this pull request as draft February 22, 2024 15:51

SakiTakamachi mentioned this pull request Feb 23, 2024

Unable to connect to MariaDB 11.3 #13452

Closed

SakiTakamachi force-pushed the fix/gh-13452 branch from f92bb21 to 75b33b2 Compare February 23, 2024 03:54

SakiTakamachi force-pushed the fix/gh-13452 branch from 75b33b2 to 0e50b96 Compare February 23, 2024 04:03

Alkarex mentioned this pull request Feb 23, 2024

MariaDB 11.3 error: Access to database is denied FreshRSS/FreshRSS#6118

Closed

SakiTakamachi commented Feb 23, 2024

View reviewed changes

SakiTakamachi changed the title ~~Fix GH-13452: Removed conn->greet_charset [mysqlnd]~~ Fix GH-13452: Fixed handshake response [mysqlnd] Feb 23, 2024

Fixed handshake response charset.

489438f

SakiTakamachi force-pushed the fix/gh-13452 branch from 0e50b96 to 489438f Compare February 29, 2024 16:03

SakiTakamachi marked this pull request as ready for review February 29, 2024 16:16

SakiTakamachi commented Feb 29, 2024

View reviewed changes

kamil-tekiela approved these changes Feb 29, 2024

View reviewed changes

SakiTakamachi closed this in e9c5f05 Mar 4, 2024

SakiTakamachi added a commit that referenced this pull request Mar 4, 2024

Merge branch 'PHP-8.2' into PHP-8.3

23d89e3

* PHP-8.2: Fixed handshake response charset. (#13470) [skip ci] Fixed NEWS

SakiTakamachi added a commit that referenced this pull request Mar 4, 2024

Merge branch 'PHP-8.3'

f799d6f

* PHP-8.3: NEWS Fixed handshake response charset. (#13470) Fixed handshake response charset. (#13470) [skip ci] Fixed NEWS

SakiTakamachi deleted the fix/gh-13452 branch March 15, 2024 14:16

Fix GH-13452: Fixed handshake response [mysqlnd] #13470

Fix GH-13452: Fixed handshake response [mysqlnd] #13470

Uh oh!

Conversation

SakiTakamachi commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Feb 22, 2024

Uh oh!

mvorisek Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

grooverdan Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

nielsdos commented Feb 22, 2024

Uh oh!

SakiTakamachi commented Feb 22, 2024

Uh oh!

kamil-tekiela commented Feb 22, 2024

Uh oh!

SakiTakamachi commented Feb 23, 2024

Uh oh!

SakiTakamachi commented Feb 23, 2024

Uh oh!

kamil-tekiela commented Feb 23, 2024

Uh oh!

SakiTakamachi commented Feb 23, 2024

Uh oh!

SakiTakamachi commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Feb 23, 2024

Uh oh!

SakiTakamachi commented Feb 23, 2024

Uh oh!

SakiTakamachi Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela commented Feb 23, 2024

Uh oh!

kamil-tekiela commented Feb 23, 2024

Uh oh!

SakiTakamachi commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Feb 25, 2024

Uh oh!

kamil-tekiela commented Feb 25, 2024

Uh oh!

SakiTakamachi commented Feb 26, 2024

Uh oh!

SakiTakamachi commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grooverdan commented Feb 29, 2024

Uh oh!

SakiTakamachi commented Feb 29, 2024

Uh oh!

SakiTakamachi Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela left a comment

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Feb 29, 2024

Uh oh!

Uh oh!

SakiTakamachi commented Feb 22, 2024 •

edited

Loading

SakiTakamachi commented Feb 23, 2024 •

edited

Loading

SakiTakamachi commented Feb 23, 2024 •

edited

Loading

SakiTakamachi commented Feb 28, 2024 •

edited

Loading

SakiTakamachi Feb 29, 2024 •

edited

Loading