MDEV-35715: UBSAN: runtime error: 1e+19 is outside the range of representable values of type 'long long' in Field_bit::store on INSERT#5081
Conversation
c20fff0 to
a6268f5
Compare
There was a problem hiding this comment.
Code Review
This pull request performs extensive security hardening and bug fixes, notably replacing sprintf with snprintf across the codebase and the CONNECT storage engine to prevent buffer overflows. Key enhancements include strict numeric validation for master.info, LENENC support for long passwords in COM_CHANGE_USER, and improved handling of negative zero in floating-point storage. It also resolves various crash scenarios in GIS and JSON functions, memory leaks, and replication inconsistencies. Review feedback identified a critical type mismatch in sql/wsrep_applier.cc that could lead to undefined behavior during debug logging, and a potential stack buffer overflow in sql/mysqld.cc when handling Unix socket paths.
I am having trouble creating individual review comments. Click here to see my feedback.
sql/wsrep_applier.cc (103-104)
The change from a byte buffer to a std::vectorstd::string (implied by push_back(err_str)) seems to conflict with the subsequent WSREP_DEBUG call (line 106-108 in context) which uses dst.data() with a %s format specifier. If dst is now a vector of strings, dst.data() returns a std::string*, which will cause Undefined Behavior or a crash when passed to %s. Please ensure the debug print is updated to access the string content correctly, e.g., dst.empty() ? "" : dst[0].c_str().
sql/mysqld.cc (2707)
The use of strmov to copy path into addr.sun_path is potentially dangerous if path exceeds the fixed size of the sun_path array (typically 108 bytes). While there is a length check later in network_init, this utility function should ideally be self-protecting. Consider using strmake or adding an explicit length check before the copy to prevent a stack buffer overflow.
if (strlen(path) >= sizeof(addr.sun_path))
{
sql_print_error("Unix socket path is too long: %s", path);
unireg_abort(1);
}
strmov(addr.sun_path, path);|
Lets get a working local test environment so mtr matches results before submitting. |
a6268f5 to
0fbd468
Compare
f51a3ce to
b7ae0fb
Compare
Handle the two cases not covered by Field_bit::store(const char*): - nr < 0: clamp to 0 and emit ER_WARN_DATA_OUT_OF_RANGE - nr >= 2^64: clamp to ULLONG_MAX and emit ER_WARN_DATA_OUT_OF_RANGE All other range checks are handled correctly by the existing store(const char*, size_t, CHARSET_INFO*) implementation.
b7ae0fb to
4163676
Compare
| return Field_bit::store((longlong) nr, FALSE); | ||
| DBUG_ASSERT(marked_for_write_or_computed()); | ||
| if (nr < 0) | ||
| { |
There was a problem hiding this comment.
like Field_bit::store(const char * lets do error if thd->really_abort_on_warning() and warning otherwise. The implementation of this corresponds to the sql_mode=STRICT_ALL_TABLES which can form part of the tests.
| set_warning(Sql_condition::WARN_LEVEL_WARN, ER_WARN_DATA_OUT_OF_RANGE, 1); | ||
| return Field_bit::store(0LL, TRUE); | ||
| } | ||
| if (nr >= 18446744073709551616.0) /* 2^64 */ |
| if (nr >= 18446744073709551616.0) /* 2^64 */ | ||
| { | ||
| set_warning(Sql_condition::WARN_LEVEL_WARN, ER_WARN_DATA_OUT_OF_RANGE, 1); | ||
| return Field_bit::store((longlong)ULLONG_MAX, TRUE); |
There was a problem hiding this comment.
Comment here above return on why there a case to (signed) long long, referring to TRUE meaning its processed as unsigned.
| { | ||
| set_warning(Sql_condition::WARN_LEVEL_WARN, ER_WARN_DATA_OUT_OF_RANGE, 1); | ||
| return Field_bit::store((longlong)ULLONG_MAX, TRUE); | ||
| } |
There was a problem hiding this comment.
You're casting twice below. Just (ulonglong) cast is sufficient. I note that (longlong) along generates the UBSAN error we aimed to avoid.
| DELETE FROM t1; | ||
|
|
||
| --echo # BIT(1) - truncation/rounding behaviour | ||
| INSERT INTO t1 VALUES (0.1); |
There was a problem hiding this comment.
could simplify with a single insert statement of all 3 values and omit the DELETE statements, including the one before the DROP TABLE.
| INSERT INTO t3 VALUES (0.0); | ||
| INSERT INTO t3 VALUES (1.5); | ||
| --error ER_WARN_DATA_OUT_OF_RANGE | ||
| INSERT INTO t3 VALUES (-1.0); |
There was a problem hiding this comment.
There's not a test case here that populates the 64bits. or overflows it. Please add these values.
Note the func_math has a set of integer tests that populate the bit fields if you want to mirror it changing integer value to double. I do like the HEX representation here however for the output.
| INSERT INTO t2 VALUES (127.6); | ||
| INSERT INTO t2 VALUES (255.0); | ||
| --error ER_DATA_TOO_LONG | ||
| INSERT INTO t2 VALUES (256.0); |
There was a problem hiding this comment.
To produce a warning on these to show that its sql_mode dependent
set statement sql_mode='' FOR INSERT INTO t2 VALUES (256.0); will result in a warning instead of errro. Include this form below the --error statements and this will show both the warning and the truncation.
| --error ER_WARN_DATA_OUT_OF_RANGE | ||
| INSERT INTO t3 VALUES (-1.0); | ||
| SELECT HEX(c) FROM t3; | ||
| DROP TABLE t3; |
There was a problem hiding this comment.
Move this test case to the bottom of main/func_math.test.
End with a --echo # end of 10.11 tests.
| @@ -0,0 +1,40 @@ | |||
| --echo # MDEV-35715: UBSAN runtime error in Field_bit::store on INSERT | |||
|
|
|||
There was a problem hiding this comment.
include a --echo # before and after the header.
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Couple of things:
- your PR explanation does not match what you did.
- you only cover bit. Neither enum nor set, as mentioned in the PR. Please add coverage for these.
| set_warning(Sql_condition::WARN_LEVEL_WARN, ER_WARN_DATA_OUT_OF_RANGE, 1); | ||
| return Field_bit::store(0LL, TRUE); | ||
| } | ||
| if (nr >= 18446744073709551616.0) /* 2^64 */ |
| int Field_bit::store(double nr) | ||
| { | ||
| return Field_bit::store((longlong) nr, FALSE); | ||
| DBUG_ASSERT(marked_for_write_or_computed()); |
There was a problem hiding this comment.
use the utility class instead!
When a floating-point value (e.g.,
1e+19or-1e+30) that is outside the representable range of a 64-bit integer type is cast directly via(longlong) nror(ulonglong) nr, C++ produces Undefined Behavior (UB). UndefinedBehaviorSanitizer (UBSAN) flags this during runtime as afloat-cast-overflow.This directly affected:
Field_bit::store(double)Field_enum::store(double)Field_set::store(double)(inlined toField_enum::store)Solution
Replaced the direct C-style
(longlong)casts with MariaDB's standardConverter_double_to_longlonghelper class.Instead of an unsafe cast, the converter safely rounds and clamps out-of-bounds
doublevalues to their respective integer minimums/maximums (ULLONG_MAX,LLONG_MAX, orLLONG_MINdepending on the explicitunsigned_flag) before proceeding with the normal truncation logic. This replicates the exact behavior existing in other numericstore(double)methods, maintaining the same warning outputs (ER_WARN_DATA_OUT_OF_RANGE/WARN_DATA_TRUNCATED) while completely eliminating the Undefined Behavior.Testing
main.type_bit_mdev35715validatingBIT,ENUM, andSEToverflow boundaries.