MDEV-39198 Reject empty string for startup options to mariadbd#5246
MDEV-39198 Reject empty string for startup options to mariadbd#5246y2kwak wants to merge 1 commit into
Conversation
Add an explicit empty-string check to reject startup options with empty values. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation to reject empty values for GET_SET options, along with corresponding test cases. The review feedback identifies a potential null pointer dereference in mysys/my_getopt.c where argument is dereferenced as argument[0] without checking if argument is NULL first.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!argument[0]) | ||
| { | ||
| res= EXIT_ARGUMENT_INVALID; | ||
| goto ret; | ||
| } |
There was a problem hiding this comment.
Dereferencing argument without checking if it is NULL can lead to a segmentation fault if the option is processed without an argument. To prevent a potential null pointer dereference, ensure argument is validated before checking its first character. Note that validation logic for configuration variables should explicitly allow empty strings if they are considered valid or represent the default state, so we should only check for NULL here.
if (!argument)
{
res= EXIT_ARGUMENT_INVALID;
goto ret;
}References
- Validation logic for configuration variables should explicitly allow empty strings if they are considered valid or represent the default state.
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
FYI, some tests are failing after your change, e.g.:
main.show_check w6 [ fail ]
Test ended at 2026-06-15 19:28:26
CURRENT_TEST: main.show_check
Failed to start mysqld.1
mysqltest failed but provided no output
worker[06] > Restart - not started
- saving '/home/buildbot/aarch64-debian-11/build/mysql-test/var/6/log/main.show_check/' to '/home/buildbot/aarch64-debian-11/build/mysql-test/var/log/main.show_check/'
Retrying test main.show_check, attempt(2/3)...
***Warnings generated in error logs during shutdown after running tests: main.show_check
2026-06-15 19:28:26 0 [ERROR] /home/buildbot/aarch64-debian-11/build/sql/mariadbd: Error while setting value '' to 'myisam-recover-options'
2026-06-15 19:28:26 0 [ERROR] Parsing options for plugin 'MyISAM' failed. Disabling plugin
2026-06-15 19:28:26 0 [ERROR] Failed to initialize plugins.
2026-06-15 19:28:26 0 [ERROR] Aborting
Also, Since this is a bug fix, I'd suggest you consider fixing this into the lowest affected GA version, I'd guess 10.11 in this case. I'll leave the backport for the final reviewer to decide on (it might be risky), but IMHO it should be done.
3 participants
Description
Previously, empty value start up commands were accepted silently, configuring the variable to default value. This change adds an explicit empty-string check to reject startup options with empty values.
How can this PR be tested?
Before the change,
--create-tmp-table-binlog-formats=''is acceptedAfter the change, an error is logged at start up.
MTR test case is added to existing 'mysqld_option_err' test.
Basing the PR against the correct MariaDB version
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.