MDEV-40647 OOB read in IO Thread if the FDEv does not support Rotate Events - #5505
MDEV-40647 OOB read in IO Thread if the FDEv does not support Rotate Events#5505ParadoxV5 wants to merge 3 commits into
Conversation
* The crafted invalid FDEs were omitted their checksums, even though
the base code (still) adds the checksum length to the event length.
This commit fixes this discrepancy by not skipping the event footer
step (write the checksum, and finish up encryption if active),
so the fault injections are more self-contained.
This discrepancy did not matter in practice because
* The event loading simply assumes the first
few bytes of the next event as the unused checksum.
* The fix to `get_checksum_alg()` is detecting invalidity before the
code reaches the fixed parser-contructor.
This is rather an implementation detail, though, as the constructor
fix would come to effect if we refactor `get_checksum_alg()` away.
* This commit also disables echoing `SHOW BINLOG EVENTS IN`
to the results in case the `$binlog_file` is not consistent.
It does not typically fail, but should trip MSAN.
…Events If the replication IO Thread receives a Rotate event following a Format Description event (FDE) with no post-header length for Rotate events, the Rotate event’s parser constructor indexes the FDE’s post-header lengths array out of bounds. This commit defends against this situation by checking before the constructor that the FDE describes Rotate events as recognized at all. In practice, because the Binlog Dump thread generates a Fake `ROTATE_EVENT` **before** sending the FDE, it has pinned Rotate events’ post-header length to 8 regardless of FDEs. This fix solution considers that the FDE’s description should still be respected, matching the constructor. Reviewed-by: Kristian Nielsen <knielsen@knielsen-hq.org>
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
I’m still not fully certain about whether I should always consider the post-header length as pinned; that is, ignore whatever the FDE says (or complain if it does not match).
OTOH, if the post-header length is to be used, then it’s another bug that the constructor should uses uint8korr() directly on the buffer to read the log position: this could overrun into the log name or even past the buffer.
Actually, why is the Fake ROTATE_EVENT before the FDE in the first place?
Can we fix this ordering, especially after MDEV-38906 gets rid of IO Thread’s feeble mid-group continuation?
| @@ -0,0 +1,51 @@ | |||
| # MDEV-40647 OOB read in IO Thread if the FDEv does not support Rotate Events | |||
|
|
|||
| --source include/have_debug.inc | |||
There was a problem hiding this comment.
MDEV-40647 requires MSAN to reproduce in CI, but our MSAN CI does not have_debug.
(The alternative is to upload the suspicious binlog the fault injection generates…
Do we want this approach instead?)
| DBUG_EVALUATE_IF("truncate_fde_post_header_len", 0, number_of_event_types) + | ||
| DBUG_EVALUATE_IF("truncate_fde_used_checksum_alg", 0, | ||
| BINLOG_CHECKSUM_ALG_DESC_LEN); |
There was a problem hiding this comment.
I debated with myself whether to simply set some X_size local variables to 0,
but decided that skipping entire write_data() calls is more explicit,
especially considering that an explicit size parameter doesn’t normally make sense for numeric fields.
| { | ||
| /* | ||
| It's normally done in Log_event::read_log_event(), | ||
| but we bypass it here because it's expensive and costs dynamic memory. |
There was a problem hiding this comment.
The design oversight behind this is that the IO Thread needs to preprocess certain events, but using Log_event::read_log_event() uniformly is unnecessarily expensive, especially for the events it doesn’t preprocess.
ParadoxV5
changed the title
2 participants
This PR is based on #5419 (MDEV-40365/MDEV-40366) for merging their fault injections.
Additionally, the first commit of this PR is a post-approval fixup for #5419; details are in its commit message.
If the replication IO Thread receives a Rotate event following a Format Description event (FDE) with no post-header length for Rotate events, the Rotate event’s parser constructor indexes
the FDE’s post-header lengths array out of bounds.
This commit defends against this situation by checking before the constructor that the FDE describes Rotate events as recognized at all.
(TODO: squash commits 2 into 3)