MDEV-40147 MSAN: use-of-uninitialized-value in my_time_compare#5281
MDEV-40147 MSAN: use-of-uninitialized-value in my_time_compare#5281grooverdan wants to merge 1 commit into
Conversation
Field::get_date and Item::get_date can return true and not have their ltime pointer structure initialized. This was the case with Field_vers_trx_id::get_date. Type_handler_time_common::stored_field_cmp_to_item rather than doing a my_time_compare on uninitialized values can return a non-equality (!=0) value for when the fetching of Field or Item fails and avoid the call to my_time_compare. Added checks for other invalid results before being passed to my_time_compare. Add nonnull as an attribute on my_time_compare and the unpack_time function it used. Also pack_time since related and it obviously requires a non-null argument.
There was a problem hiding this comment.
Code Review
This pull request addresses an MSAN uninitialized value issue in my_time_compare (MDEV-40147) by adding __attribute__((nonnull)) to temporal functions, checking return values of get_date before comparisons, and adding a regression test. The review feedback points out an inconsistency in Type_handler_temporal_with_date::stored_field_cmp_to_item where returning 1 when field->get_date fails is incorrect and asymmetric, suggesting it should return -1 instead to match similar comparison handlers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (field->get_date(&field_time, Datetime::Options(TIME_INVALID_DATES, thd)) | ||
| || item->get_date(thd, &item_time, Datetime::Options(TIME_INVALID_DATES, thd)) | ||
| || (item_time.time_type == MYSQL_TIMESTAMP_TIME && | ||
| time_to_datetime(thd, &item_time, item_time_cmp= &item_time2))) | ||
| return 1; |
There was a problem hiding this comment.
In Type_handler_temporal_with_date::stored_field_cmp_to_item, if field->get_date fails, returning 1 (which indicates field > item) is inconsistent and incorrect when item is valid. To be consistent with Type_handler_time_common::stored_field_cmp_to_item (lines 9124-9127), we should return -1 when field->get_date fails, and 1 when item->get_date (or its conversion) fails. This ensures symmetric and correct comparison behavior when one of the operands is invalid.
if (field->get_date(&field_time, Datetime::Options(TIME_INVALID_DATES, thd)))
return -1;
if (item->get_date(thd, &item_time, Datetime::Options(TIME_INVALID_DATES, thd)) ||
(item_time.time_type == MYSQL_TIMESTAMP_TIME &&
time_to_datetime(thd, &item_time, item_time_cmp= &item_time2)))
return 1;
Field::get_date and Item::get_date can return true and not have their ltime pointer structure initialized.
This was the case with Field_vers_trx_id::get_date.
Type_handler_time_common::stored_field_cmp_to_item rather than doing a my_time_compare on uninitialized values can return a non-equality (!=0) value for when the fetching of Field or Item fails and avoid the call to my_time_compare.
Added checks for other invalid results before being passed to my_time_compare.
Add nonnull as an attribute on my_time_compare and the unpack_time function it used. Also pack_time since related and it obviously requires a non-null argument.