MDEV-37952: Fix crash when setting mroonga_default_tokenizer to NULL

[hadeer-r]

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:

• Added a check for NULL pointers in the variable assignment logic.
• Mapped NULL input to the "off" behavior to allow users to reset the tokenizer safely without crashing the server.
• Added mtr test to validate behavior when setting mroonga_default_tokenizer to null

---

Fix: MDEV-37952

[CLAassistant]

All committers have signed the CLA.

[kou]

+1

[hadeer-r]

I have applied the feedback to use DEFAULT and empty line formatting.

[hadeer-r]

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]

Thank you for your contribution! This is a preliminary review.

[gkodinov]

Optional (as I realize most of the engine's string parameters are already behaving this way): I believe I do not quite like the silent substitution of NULL with "off". IMHO, if you don't want NULLs as arguments, then reject these.

[hadeer-r]

I understand your concern about silent substitution. My understanding is that "off" means the default tokenizer is set to NULL (disabled). So when a user passes NULL, it seems appropriate to treat it as an intention to disable the tokenizer by setting the value to "off", as suggested by @kou

[kou]

Yes. As a Mroonga maintainer, I choose this approach.

[gkodinov]

you are going to call mrn_my_strdup() on this value below. Do you need to call it twice?

[hadeer-r]

I think it will not be called twice. From my understanding, when the macro MRN_NEED_FREE_STRING_MEMALLOC_PLUGIN_VAR exists, it means we need to free the variable before allocating a new one, and this happens in:

#ifdef MRN_NEED_FREE_STRING_MEMALLOC_PLUGIN_VAR
my_free(*old_value_ptr);
*old_value_ptr = mrn_my_strdup(new_value, MYF(MY_WME));

Otherwise, when the macro is off, it will not allocate the new value "off" which is in the static area, so when trying to free it later, it will cause a segmentation fault. This is in the MySQL case, so should I remove it? As the edit is in MariaDB, I updated it to be synchronized with Mroonga and to be consistent with other functions.

[gkodinov]

You're allocating the new_value! That's not yet in old_value_ptr!

Also note that plugin_vars_free_values() will always my_free the value assigned anyway for all PLUGIN_VAR_MEMALLOC variables (such as this one). Thus, assigning a constant string literal So, I'd guess that it won't work sans MRN_NEED_FREE_STRING_MEMALLOC_PLUGIN_VAR. But please don't take my word for it. It looks like you never compile without it anyway.

[kou]

(Note that this code is not for MariaDB as hadeer-r already mentioned.)

It seems that you missed #ifndef and #ifdef differences.

This code is pre-processed like the following in MariaDB:

  if(!new_value) {
    new_value = "off";
  }
  // ...
  my_free(*old_value_ptr);
  *old_value_ptr = mrn_my_strdup(new_value, MYF(MY_WMF));

This code is pre-processed like the following in MySQL:

  if(!new_value) {
    new_value = "off";
    new_value = mrn_my_strdup(new_value, MYF(MY_WME));
  }
  // ...
  *old_value_ptr = (char *)new_value;

mrn_my_strdup() is never called twice.