Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ext/mysqlnd/mysqlnd_charset.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
#ifndef MYSQLND_CHARSET_H
#define MYSQLND_CHARSET_H

#define MYSQLND_UTF8_MB3_DEFAULT_ID 33
#define MYSQLND_UTF8_MB4_DEFAULT_ID 45

PHPAPI zend_ulong mysqlnd_cset_escape_quotes(const MYSQLND_CHARSET * const charset, char * newstr,
const char * escapestr, const size_t escapestr_len);

Expand Down
14 changes: 7 additions & 7 deletions ext/mysqlnd/mysqlnd_commands.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "mysqlnd_auth.h"
#include "mysqlnd_wireprotocol.h"
#include "mysqlnd_debug.h"
#include "mysqlnd_charset.h"


/* {{{ mysqlnd_command::set_option */
Expand Down Expand Up @@ -642,13 +643,12 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!conn->greet_charset) {
char * msg;
mnd_sprintf(&msg, 0, "Server sent charset (%d) unknown to the client. Please, report to the developers", greet_packet.charset_no);
SET_CLIENT_ERROR(conn->error_info, CR_NOT_IMPLEMENTED, UNKNOWN_SQLSTATE, msg);
mnd_sprintf_free(msg);
goto err;
const MYSQLND_CHARSET *read_charset = mysqlnd_find_charset_nr(greet_packet.charset_no);
if (!read_charset) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@SakiTakamachi SakiTakamachi Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

greet_packet.charset_no = conn->m->get_server_version(conn) >= 50500 ? MYSQLND_UTF8_MB4_DEFAULT_ID : MYSQLND_UTF8_MB3_DEFAULT_ID;
conn->greet_charset = mysqlnd_find_charset_nr(greet_packet.charset_no);
} else {
conn->greet_charset = read_charset;
}

conn->server_capabilities = greet_packet.server_capabilities;
Expand Down