MDEV-39240 10.6-11.4 Replication Allows Full Range for 32-bit Unsigned Timestamps#4933
MDEV-39240 10.6-11.4 Replication Allows Full Range for 32-bit Unsigned Timestamps#4933
Conversation
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
This branch is based on Community Server 10.11 as CS 10.6 support ends soon.
Enterprise Server 10.6 will also receive this fix.
Let me know if CS 10.6 should also receive this patch.
There was a problem hiding this comment.
Per the discussion on Zoom, this should error even with @@slave_type_conversions=ALL_LOSSY enabled (instead of clamping to Y2038), because ‘lossy’ is intended more for “(decimal) precision” than “range maximum”.
I still put the test in @@slave_type_conversions’s script, not only to utilize suite/rpl/include/check_type.inc, but also to assert that @@slave_type_conversions doesn’t change the treatment.
There was a problem hiding this comment.
…
However, variable substitution for SQL commands can’t work with strings containing both ' and ", which affects suite/rpl/include/check_type.inc (and others, such as include/write_var_to_file.inc), so I switched to Last_SQL_Errno instead.
(Help thread: Escaping quotes for --eval in MTR)
| DBUG_EXECUTE_IF("rpl_pack_simulate_negation", | ||
| for (uchar *byte= old_pack_ptr; byte < pack_ptr; ++byte) | ||
| *byte= ~*byte; | ||
| ); |
There was a problem hiding this comment.
I considered sneaking over-max timestamps into the primary server, but I’m concerned that it’s too UB from reading the relevant entry code, and so decided not to fix any tangential problems this hack surfaces.
| { | ||
| if (likely(seconds >= 0 && seconds <= TIMESTAMP_MAX_VALUE)) | ||
| break; | ||
| else if (likely(seconds == UINT_MAX32)) // They are both signed. |
There was a problem hiding this comment.
my_time_t and UINT_MAX32 are both signed before MDEV-32188 (11.5.1).
(-1’s equivalence is only de facto.)
|
Failures |
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Thanks for a clean patch @ParadoxV5 !
I left a couple very minor notes to address. Also before pushing this, let's have Deepthi do some additional testing on it as well.
| */ | ||
| #if !defined DBUG_OFF && defined DBUG_TRACE | ||
| const uchar *old_pack_ptr= pack_ptr; | ||
| #ifndef DBUG_OFF |
There was a problem hiding this comment.
Is this just cleanup? The patch should be null-merged into 11.8, so let's keep it as minimal as possible. Though you can do the cleanup as a separate patch if you'd like.
There was a problem hiding this comment.
It’s to be careful: old_pack_ptr was previously only used for a DBUG_PRINT, but is now also used for my new DBUG_EXECUTE_IF block.
| char unixtime[sizeof(unixtime_format) + 12]; | ||
| snprintf(unixtime, sizeof(unixtime), unixtime_format, | ||
| seconds, microseconds); | ||
| rgi->rli->report(ERROR_LEVEL, ER_TRUNCATED_WRONG_VALUE_FOR_FIELD, |
There was a problem hiding this comment.
In the commit message where you say "error on the others" it would be good to extend it with the actual error that is thrown.
There was a problem hiding this comment.
I’ve reworded along with a base sync (and to rerun CI while here).
…d Timestamps Row-based Replication did not validate serialized timestamps, which allows (pre-11.5) slaves to accept “negative” timestamps (between the Year 2038 limit and the 4-byte limit) via replication. In particular, a completely valid source of those timestamps is 11.5.1+ masters, where MDEV-32188 has increased the upper limit of timestamps to Year 2106 by utilizing this previously invalid range. This commit fills in this pre-processing: swap the Year 2106 max with the Year 2038 max, or throw an “Incorrect timestamp value” error for the others. Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>
2 participants
Row-based Replication did not validate serialized timestamps, which allows (pre-11.5) slaves to accept “negative” timestamps (between the Year 2038 limit and the 4-byte limit) via replication.
In particular, a completely valid source of those timestamps is 11.5.1+ masters, where MDEV-32188 has increased the upper limit of timestamps to Year 2106 by utilizing this previously invalid range.
This commit fills in this pre-processing: swap the Year 2106 max with the Year 2038 max, or throw an “Incorrect timestamp value” error for the others.