MDEV-37952: Fix crash when setting mroonga_default_tokenizer to NULL#4606
MDEV-37952: Fix crash when setting mroonga_default_tokenizer to NULL#4606vuvova merged 4 commits intoMariaDB:10.6from
Conversation
|
|
storage/mroonga/mysql-test/mroonga/storage/t/variable_default_tokenizer_disable.test
Outdated
Show resolved
Hide resolved
|
I have applied the feedback to use DEFAULT and empty line formatting. |
gkodinov
added
the
External Contribution
- This PR fixes a SIGSEGV crash occurring when the system variable mroonga_default_tokenizer is set to NULL. - The crash was caused by passing a NULL pointer to strcmp() in the update function of the mroonga_default_tokenizer variable. **Changes** - Mapped NULL input to "off" behavior. - Included a test case to verify the fix. This is the same fix I submitted to MariaDB here: MariaDB/server#4606 --------- Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
|
Hi, I have applied the manual allocation changes for new_value in mrn_default_tokenizer_update as discussed, aligning with the memory management pattern used in the previous Mroonga PR. |
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.
storage/mroonga/mysql-test/mroonga/storage/t/variable_default_tokenizer_disable.test
Show resolved
Hide resolved
storage/mroonga/mysql-test/mroonga/storage/t/variable_default_tokenizer_disable.test
Show resolved
Hide resolved
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
The preliminary review is now done. Please wait for the final review.
|
for the record: the review from the domain experts for this codebase are good enough. Will push (after checking with @vuvova). |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
One last thing before we merge this: Can you re-base to 10.6 please? This is a crashing bug and the jira says it applies to 10.6 onwards.
|
sure, I will rebase it now |
|
I noticed that rebasing onto 10.6 is trying to replay the entire history (over 3000 commits) because my branch was based on a newer version. My plan is to create a fresh branch from 10.6, cherry-pick only my specific commits, and then force push to update this PR. Does this approach look good? |
Yes, that is the easiest way indeed. |
Setting mroonga_default_tokenizer to NULL caused a server crash because the update function did not handle the NULL value before passing it to strcmp. Handle NULL values by treating them as "off" to allow safe variable reset.
Align with existing mroonga memory management patterns to ensure consistency, rather than relying on the MariaDB framework default behavior.
Add copyright header to the test file. Disable the test in embedded mode by sourcing include/not_embedded.inc."
|
I noticed the AppVeyor CI check is failing with: I've rebased my branch on top of the latest 10.6 and verified locally that there are no merge conflicts. |
5 participants
Summary
This PR fixes a SIGSEGV crash occurring when the system variable mroonga_default_tokenizer is set to NULL. The fix introduces a safety check to handle NULL values gracefully by treating them as an "off" state, preventing invalid memory access during string comparison.
The crash was rooted in the update function for the mroonga_default_tokenizer variable. It lacked a validation step for NULL inputs before passing the value to strcmp().
Key Changes:
Fix: MDEV-37952