Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

MDEV-36316/MDEV-36327/MDEV-36328 Debug msan fixes 10.6 #3899

Open
wants to merge 4 commits into
base: 10.6
Choose a base branch
from
+22 −6
Open

MDEV-36316/MDEV-36327/MDEV-36328 Debug msan fixes 10.6 #3899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ee600b
MDEV-36316 Debug MSAN error on InnoDB Bootstrap
grooverdan Mar 19, 2025
7aa79ec
default stack size under MSAN needs increasing
grooverdan Mar 19, 2025
f518ef4
MDEV-36327 MSAN use-of-uninitialized-value in i_s_sys_columns_fill_table
grooverdan Mar 19, 2025
5e9b106
MDEV-36328 MSAN: use-of-uninitialized-value in ha_innobase::records_i…
grooverdan Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion include/my_pthread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,17 @@ extern void my_mutex_end(void);
We need to have at least 256K stack to handle calls to myisamchk_init()
with the current number of keys and key parts.
*/
# if defined(__SANITIZE_ADDRESS__) || defined(WITH_UBSAN)
#if !defined(__has_feature)
#define __has_feature(x) 0
#endif
#if defined(__clang__) && __has_feature(memory_sanitizer) && !defined(DBUG_OFF)
/*
MSAN in Debug with clang-20.1 required more memory to complete
mtr begin/end checks. The result without increase was MSAN
errors triggered on a call instruction.
*/
# define DEFAULT_THREAD_STACK (448*1024L) /* 458752 */
# elif defined(__SANITIZE_ADDRESS__) || defined(WITH_UBSAN)
/*
Optimized WITH_ASAN=ON executables produced
by GCC 12.3.0, GCC 13.2.0, or clang 16.0.6
Expand Down
4 changes: 2 additions & 2 deletions storage/innobase/handler/ha_innodb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14476,8 +14476,8 @@ ha_innobase::records_in_range(
n_rows = rtr_estimate_n_rows_in_range(
index, range_start, mode1);
} else {
btr_pos_t tuple1(range_start, mode1, pages->first_page);
btr_pos_t tuple2(range_end, mode2, pages->last_page);
btr_pos_t tuple1(range_start, mode1, 0);
btr_pos_t tuple2(range_end, mode2, 0);
Comment on lines -14479 to +14480
Copy link
Contributor

Choose a reason for hiding this comment

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

This would seem to be the actual fix. ~0ULL might be a safer value, but I think that 0 should be OK as well, because the smallest possible index page number is 3.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I could add MEM_UNDEFINED(&tuple1.page_id, sizeof tuple1.page_id) (plus tuple2) to protect against future btr_estimate_n_rows_in_range that access it.

n_rows = btr_estimate_n_rows_in_range(index, &tuple1, &tuple2);
pages->first_page= tuple1.page_id.raw();
pages->last_page= tuple2.page_id.raw();
Expand Down
6 changes: 3 additions & 3 deletions storage/innobase/handler/i_s.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5453,7 +5453,7 @@ i_s_dict_fill_sys_columns(
const char* col_name, /*!< in: column name */
dict_col_t* column, /*!< in: dict_col_t struct holding
more column information */
ulint nth_v_col, /*!< in: virtual column, its
ulint* nth_v_col, /*!< in: virtual column, its
sequence number (nth virtual col) */
Comment on lines -5456 to 5457
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand why we would need any of the changes to this file and which problem these changes would solve. We’re no longer passing a read-only parameter by value but via a pointer that is effectively read-only. Can you test again without including any of these changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

The error received was in MDEV-36327.

It was working around a msan patten of possibly not tracing undefined memory once it got copied to registers and then used as argument to the next function.

An alternative is:

$ git diff storage/innobase/dict/dict0load.cc
diff --git a/storage/innobase/dict/dict0load.cc b/storage/innobase/dict/dict0load.cc
index 87ff163a233..0b2cdc9e01b 100644
--- a/storage/innobase/dict/dict0load.cc
+++ b/storage/innobase/dict/dict0load.cc
@@ -1184,6 +1184,8 @@ static const char *dict_load_column_low(dict_table_t *table,
        /* Report the virtual column number */
        if ((prtype & DATA_VIRTUAL) && nth_v_col != NULL) {
                *nth_v_col = dict_get_v_col_pos(pos);
+       } else {
+               *nth_v_col = 0;
        }
 
        return(NULL);

TABLE* table_to_fill) /*!< in/out: fill this table */
{
Expand All @@ -5468,7 +5468,7 @@ i_s_dict_fill_sys_columns(
OK(field_store_string(fields[SYS_COLUMN_NAME], col_name));

if (column->is_virtual()) {
ulint pos = dict_create_v_col_pos(nth_v_col, column->ind);
ulint pos = dict_create_v_col_pos(*nth_v_col, column->ind);
OK(fields[SYS_COLUMN_POSITION]->store(pos, true));
} else {
OK(fields[SYS_COLUMN_POSITION]->store(column->ind, true));
Expand Down Expand Up @@ -5535,7 +5535,7 @@ i_s_sys_columns_fill_table(
if (!err_msg) {
err = i_s_dict_fill_sys_columns(
thd, table_id, col_name,
&column_rec, nth_v_col,
&column_rec, &nth_v_col,
tables->table);
if (err) {
err = i_s_sys_error_handling(err, thd);
Expand Down
6 changes: 6 additions & 0 deletions storage/innobase/include/rem0rec.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ rec_set_bit_field_1(
ut_ad(((mask >> shift) << shift) == mask);
ut_ad(((val << shift) & mask) == (val << shift));

#ifndef DBUG_OFF
MEM_MAKE_DEFINED(rec - offs, 1);
#endif
mach_write_to_1(rec - offs,
(mach_read_from_1(rec - offs) & ~mask)
| (val << shift));
Comment on lines +159 to 164
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks incorrect to me. Why would we claim that all bits at rec[-offs] are initialized when we are only overwriting some of the bits here? What would fail if this change and the similar change to rec_set_bit_field_2() were omitted?

Copy link
Member Author

Choose a reason for hiding this comment

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

The trace was like in MDEV-36316.

I agree it seem overly incorrect claiming all bits are initialised. It only occurred in Debug mode so I'm assuming a less optimised code makes this look different. I'll look at forcing a higher optimisation on these blocks as an alternative.

Expand Down Expand Up @@ -198,6 +201,9 @@ rec_set_bit_field_2(
ut_ad(((mask >> shift) << shift) == mask);
ut_ad(((val << shift) & mask) == (val << shift));

#ifndef DBUG_OFF
MEM_MAKE_DEFINED(rec - offs, 2);
#endif
mach_write_to_2(rec - offs,
(mach_read_from_2(rec - offs) & ~mask)
| (val << shift));
Expand Down