MySQL Bugs: #116752: Setting MYSQL_COMMAND_LOCAL_THD

Bug #116752	Setting MYSQL_COMMAND_LOCAL_THD_HANDLE may crash the server
Submitted:	21 Nov 2024 22:42	Modified:	26 Nov 2024 11:55
Reporter:	Yura Sorokin (OCA)	Email Updates:
Status:	Won't fix	Impact on me:	None
Category:	MySQL Server: Connection Handling	Severity:	S3 (Non-critical)
Version:	8.0.40 8.4.3 9.1.0	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	Contribution

Description:
Developers who are building a new component that uses mysql_command_xxx services (mysql_command_factory, mysql_command_query, etc.) may encounter the following crash when setting 'MYSQL_COMMAND_LOCAL_THD_HANDLE' option.
Basically, the following sequence of calls may crash the server

cmd_factory_srv->init(&mysql_h);
cmd_options_srv->set(local_mysql_h, MYSQL_COMMAND_LOCAL_THD_HANDLE, nullptr);

#0  0x000055555b8c97a6 in Srv_session::get_thd (this=0x0) at /home/yura/ws/mysql-server-8.0/sql/srv_session.h:249
#1  0x000055555dd5f235 in mysql_command_services_imp::set (mysql_h=0x7fff4c000c20, option=1036, arg=0x0) at /home/yura/ws/mysql-server-8.0/sql/server_component/mysql_command_services_imp.cc:667

The problem comes from this line
https://github.com/mysql/mysql-server/blob/89e1c722476deebc3ddc8675e779869f6da654c0/sql/se...
Here 'mysql_session' is used without being checked for nulless.
'mysql_session' can be set to nullptr when 'service->open(nullptr, nullptr)' returns nullptr. And this may happen when this code is executed very early after  server startup - when 'srv_session_server_is_available()' still returns false.

How to repeat:
Write a new component ('component_local_thd_handle_test') that utilizes mysql_command_xxx services with

cmd_factory_srv->init(&mysql_h);
cmd_options_srv->set(local_mysql_h, MYSQL_COMMAND_LOCAL_THD_HANDLE, nullptr);

in the component initialization routine.

Create a simple MTR test case ('main.local_thd_handle_test') that would have
INSTALL COMPONENT 'file://component_local_thd_handle_test';
as the very first line.
Run
./mtr --debug-server main.local_thd_handle_test

Suggested fix:
Return early with true when 'service->open(nullptr, nullptr)' returns nullptr.

See the attached "suggested_fix.diff" patch.

suggested fix (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: bug116752_suggested_fix.diff (application/octet-stream, text), 641 bytes.

The patch should also work for 8.4.3 and 9.1.0.

Hello Yura Sorokin,

Thank you for the report and contribution.

regards,
Umesh

Thank you for taking the time to report this!

You're mistakenly taking the component API to be a C API.
It isn't. It's a server API. You can definitely crash the server with it. But you can also crash it if you compile a "((int *)NULL) = 12" in a component and load it. 
Expecting that the component APIs will sanitize their args is what is the wrong assumption here.
Sanitizing arguments takes resources. And should only be done when it will make a difference. In this case: not really. 

So please make sure to do your own sanitization. Especially if you are taking user input!

Georgi, I think we have a misunderstanding here.
It's not like I set deliberately something to NULL and then complain about it crashing when dereferenced.
In this call
cmd_options_srv->set(local_mysql_h, MYSQL_COMMAND_LOCAL_THD_HANDLE, nullptr);
"nullptr" has special meaning.
According to the doc
https://github.com/mysql/mysql-server/blob/89e1c722476deebc3ddc8675e779869f6da654c0/sql/se...
"If null a new internal THD will be created"
Moreover, in one of the Oracle examples ("test_services_command_services"), we have identical call
https://github.com/mysql/mysql-server/blob/89e1c722476deebc3ddc8675e779869f6da654c0/plugin...

In other words, I am making a 100% valid call with "nullptr" as a "MYSQL_COMMAND_LOCAL_THD_HANDLE" parameter and "mysql_command_services_imp::set()" implementation indeed tries to create a new internal THD for me here
https://github.com/mysql/mysql-server/blob/89e1c722476deebc3ddc8675e779869f6da654c0/sql/se...
The only problem is that sometimes this call to "service->open(nullptr, nullptr)" may fail and return "nullptr" itself. This may happen, for instance, when "INSTALL COMPONENT 'file://component_my_component'"(that sets this "MYSQL_COMMAND_LOCAL_THD_HANDLE" in its 'init()' function) is run too early. In particular when the server already started to accept connections (and we can send 'INSTALL COMPONENT') but internal session service is not yet ready (when 'srv_session_server_is_available()' still returns false).