Skip to content

MDEV-30732 : wsrep_store_key_val_for_row() may invoke memcpy() on nul… #4314

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

Jump to bottom
Open
wants to merge 1 commit into
base: 10.11
Choose a base branch
from
Open

MDEV-30732 : wsrep_store_key_val_for_row() may invoke memcpy() on nul… #4314

wants to merge 1 commit into from

Conversation

janlindstrom
Copy link
Contributor

…lptr

  • The Jira issue number for this PR is: MDEV-30732

Description

Problem was that row_mysql_read_blob_ref can return NULL in case when blob datatype is used in a key and its real value is NULL. This NULL pointer is then used in memcpy function in wsrep_store_key_val_for_row. However,
memcpy is defined so that argument 2 must not be NULL.

Fixed by adding conditions before memcpy functions so that argument 2 is always non NULL.

Release Notes

TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • [x ] This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • [x ] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • [x ] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@janlindstrom janlindstrom self-assigned this Sep 24, 2025
@janlindstrom janlindstrom requested a review from dr-m September 24, 2025 06:49
@svoj svoj added the Codership Codership Galera label Sep 24, 2025
dr-m
dr-m requested changes Oct 3, 2025
View reviewed changes
storage/innobase/handler/ha_innodb.cc Outdated
Comment on lines 6938 to 6939
ut_ad(src_start || (src_start == nullptr && true_len == 0));
if (src_start) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same pattern here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

storage/innobase/handler/ha_innodb.cc
Comment on lines 6954 to 6921
} else {
memcpy(buff, src_start, true_len);
ut_ad(src_start || (src_start == nullptr && true_len == 0));
if (src_start) {
memcpy(buff, src_start, true_len);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

While here even a register-starved ISA like IA-32 might be able to hold all of real_type, mysql_type (which occur in the if condition), src_start, and true_len in registers (without any spilling to stack), I would still make this uniform with my suggestion to the other occurrences:

			} else if (true_len) {
				memcpy(buff, src_start, true_len);
			} else {
				ut_ad(!src_start);
			}

@janlindstrom janlindstrom force-pushed the 10.11-MDEV-30732 branch 2 times, most recently from 99415e3 to 4e53039 Compare October 3, 2025 12:40
dr-m
dr-m reviewed Oct 7, 2025
View reviewed changes
storage/innobase/handler/ha_innodb.cc Outdated
Comment on lines 6677 to 6680
for (; key_part != end; key_part++) {
uchar sorted[REC_VERSION_56_MAX_INDEX_COL_LEN] = {'\0'};
uchar sorted[REC_VERSION_56_MAX_INDEX_COL_LEN];
bool part_is_null = false;
memset(sorted, 0, sizeof(sorted));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to zero-initialize all 3072 bytes of this buffer on every single iteration of this loop? The variable true_len seems to accurately keep track of the used length of this buffer. Perhaps this initialization should be replaced with MEM_UNDEFINED(sorted, sizeof sorted) in order not to hide any bugs from MemorySanitizer or the Valgrind memcheck tool.

Could wsrep_innobase_mysql_sort() avoid zero-initializing another stack-allocated buffer tmp_str and modify the sorted buffer in place? Could the function be renamed to something more descriptive, because it is unclear how a single string can be compared to itself or sorted with respect to anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think I can use in place modification because strnxfrm will expand the destination string, but I can remove zero-initialization from both because in both cases used length of the buffer is kept tracked.
I rename function to wsrep_normalize_string().

storage/innobase/handler/ha_innodb.cc
Comment on lines 6757 to 6739
if (true_len > sizeof(sorted)) {
true_len = sizeof(sorted);
true_len = sizeof(sorted);
}

/* Note that when true_len == 0 it does not mean
that data == nullptr */
if (true_len) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing else before if (true_len).

storage/innobase/handler/ha_innodb.cc Outdated
Comment on lines 6705 to 6710
ulint lenlen=0;
ulint len=0;
const byte* data=nullptr;
ulint key_len=0;
ulint true_len=0;
const CHARSET_INFO* cs;
Copy link
Contributor

Choose a reason for hiding this comment

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

These initializations make the code harder to read. It would be better to defer the declaration of each of these local variables to the spot of the first assignment. Why do we declare true_len this early at all? The branch if (part_is_null) branch that unconditionally ends in continue could declare another local variable within its own scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

storage/innobase/handler/ha_innodb.cc Outdated
Comment on lines 6791 to 6804
const CHARSET_INFO* cs;
ulint key_len;
ulint true_len;
ulint key_len=0;
ulint true_len=0;
int error=0;
ulint blob_len;
const byte* blob_data;
ulint blob_len=0;
const byte* blob_data=nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding redundant assignments to some of these local variables, can be please move the declarations to the place of the first assignment, such as the following? (By the way, the type of key_part->length seems to be uint16.)

const size_t key_len = key_part->length;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

storage/innobase/handler/ha_innodb.cc Outdated
Comment on lines 6890 to 6894
ulint true_len=0;
ulint key_len=0;
const uchar* src_start=nullptr;
int error=0;
enum_field_types real_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need true_len this early at all. The body of if (part_is_null) right below this could simply refer to key_len.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

storage/innobase/handler/ha_innodb.cc Outdated
Comment on lines 6943 to 6945
/* Note that when true_len == 0 it does not mean
that src_start == nullptr */
if (true_len) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it unnecessary obfuscation to have a redundant variable true_len here. We can simply refer to key_len here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@janlindstrom
MDEV-30732 : wsrep_store_key_val_for_row() may invoke memcpy() on nul…
fa1fec8 
…lptr

Problem was that row_mysql_read_blob_ref can return NULL
in case when blob datatype is used in a key and its real
value is NULL. This NULL pointer is then used in memcpy
function in wsrep_store_key_val_for_row. However,
memcpy is defined so that argument 2 must not be NULL.

Fixed by adding conditions before memcpy functions so
that argument 2 is always non NULL.

Additional fixes after review
- Removed unnecessary copying key data from one buffer to another.
Use original key data buffer as input and temporary buffer as output.
Extra output buffer is needed because strnxfrm might expand input buffer
contents.
- Removed unnecessary initialization of variables and move
declaration where first time needed.
- Removed unnecessary intitialization of temporary buffer because
we already keep track actual filled length.
- Remove unneccessary extra call to charset->strnxfrm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dr-m dr-m dr-m requested changes

Assignees

@janlindstrom janlindstrom

Labels

Codership Codership Galera

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@janlindstrom @dr-m @svoj