MDEV-40647 OOB read in IO Thread if the FDEv does not support Rotate Events

[ParadoxV5]

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.

---

• Fixes MDEV-40647

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)

[ParadoxV5]

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?

[ParadoxV5]

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?)

[ParadoxV5]

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.

[knielsen]

ParadoxV5 ***@***.***> writes:

MDEV-40647 OOB read in IO Thread if the FDEv does not support Rotate 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. You can view, comment on, or merge this pull request online at: #5505

Here is my review of this and prior two commits:

case ROTATE_EVENT: { + /* + It's normally done in Log_event::read_log_event(), + but we bypass it here because it's expensive and costs dynamic memory. + */ + if (unlikely(ROTATE_EVENT > + mi->rli.relay_log.description_event_for_queue->number_of_event_types)) + { + // The current FDE does not support `ROTATE_EVENT`. + error= ER_SLAVE_RELAY_LOG_WRITE_FAILURE; + goto err; + } Rotate_log_event rev(buf, checksum_alg != BINLOG_CHECKSUM_ALG_OFF ? event_len - BINLOG_CHECKSUM_LEN : event_len, mi->rli.relay_log.description_event_for_queue);

Can we instead put this check of the description_event_for_queue->number_of_event_types into that Rotate_log_event constructor? That seems to be a better place for it (rather than caller having to remember to check). Also, there seems to be a check for rev.is_valid() missing (in the original code). I'd prefer it that way. But if that's not possible/convenient for some reason, the fix itself looks ok in either case.

@ParadoxV5 commented on this pull request. 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).

I consider the whole idea of the format description event containing the post-header length a misdesign. It goes way back, and I suppose there was some intention to use this for backwards-compatible extensions, but I do not think it works well for this. The way the format description event is used to interpret the bytes of other events causes a lot of complexity and trickery in the code, always this need to try to have the correct format description event available at all times. There have been lots of bugs from this. I think your approach in this patch is ok for a 10.6 fix. Going forward, my preference is to stop relying on these post-header lenghts in the FDE and even start removing references to it, in new development. My intention is that we will not change these lengths ever again going forward, instead of relying eg. on flag bits in the individual event to mark if certain fields are available (or just the length stored in the event itself). This is how most of extensions to event formats have been done in MariaDB.

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.

I don't fully follow the exact details here, but yes, trying to use the post-header length is just generally causing complexity and grief.

Actually, why is the Fake `ROTATE_EVENT` before the FDE in the first place? Can we fix this ordering, especially after [MDEV-38906](https://jira.mariadb.org/browse/MDEV-38906) gets rid of IO Thread’s feeble mid-group continuation?

I'm not sure what the consequences are of this. Again, my preferences would be if we can instead change the code so we don't need to care about the FDE or its order wrt. other events at all... Reviewed-by: Kristian Nielsen ***@***.***>

[ParadoxV5]

Here is my review of this and prior two commits:

Thanks @knielsen,

+ /*
+ It's normally done in Log_event::read_log_event(),
+ but we bypass it here because it's expensive and costs dynamic memory.
+ */
+ if (unlikely(ROTATE_EVENT >
+ mi->rli.relay_log.description_event_for_queue->number_of_event_types))

Can we instead put this check of the
description_event_for_queue->number_of_event_types into that
Rotate_log_event constructor? That seems to be a better place for it (rather
than caller having to remember to check). Also, there seems to be a check
for rev.is_valid() missing (in the original code).

I'd prefer it that way. But if that's not possible/convenient for some
reason, the fix itself looks ok in either case.

My opinion is that both this description_event_for_queue->number_of_event_types and rev.is_valid() checks are already included in the common Log_event::read_log_event() code, and it is the fault of the anti-pattern of skipping Log_event::read_log_event() (because of “expenses”) scattered thorough the codebase that they forgot the number_of_event_types and (more importantly) is_valid() checks.

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).

The way the format description event is used to interpret the bytes of other
events causes a lot of complexity and trickery in the code, always this need
to try to have the correct format description event available at all times.
There have been lots of bugs from this.

Indeed.

Going forward, my preference is to stop relying on these post-header lenghts
in the FDE and even start removing references to it, in new development. My
intention is that we will not change these lengths ever again going forward,
instead of relying eg. on flag bits in the individual event to mark if
certain fields are available (or just the length stored in the event
itself).

I remember you mentioned the non-reliance on FDE back in the MDEV-38907 Optimistic Relay Log Crash Recovery pre-review:

+        /*FIXME:
+          Shouldn't this section, as well as TC_LOG_BINLOG::recover(),
+          replace the `fdev` passed from an outer scope with
+          what's read from the log, since they may not match?
+        */

You are correct, that is the traditional requirement.

But note that going forward, the intention is that we will no longer require
knowing the particular format description event in order to be able to read
events correctly. Meaning that the format description event constructed with
Format_description_log_event fdev(BINLOG_VERSION) will be sufficient for
reading all events that the server knows about.

I did not fully understand this back then, but now I do; thanks for explaining.

I can clearly see that having to keep the FDE around from event streaming to processing is indeed not only quite a bottleneck, but approaches the anti-pattern of a god-object.

Perhaps we can get rid of FDEs (Start Events in MariaDB 5) entirely.
We bake all header extensions in flags – or entirely new event types if we have to – that “the Replication protocol” already defines; even the base event header has a flags field with unassigned bits, allowing us to eliminate FDE’s ‹common header length› field.

This is how most of extensions to event formats have been done in
MariaDB.

I’ve seen it – the presence of both flag and flag2 in event parsing code (I forgot which piece).