MDEV-40643: Corrupt Heartbeat Log Event can Crash Slave

[bnestere]

A slave can crash when its master sends an event shorter than the
event's own header. A truncated heartbeat makes debug builds fail an
assertion in Binary_string::realloc_raw(). On 64-bit release builds it
stops the slave IO thread with an error message that omits the log
file name. If the master also declares an oversized common header in
its format description event, the slave allocates and fills nearly 4GB
instead. On 32-bit release builds it writes that error message past
the end of a stack buffer. An event under four bytes crashes every
build, because the slave checksums roughly 16 EiB and reads far past
the end of the packet.

Neither queue_event() nor the Heartbeat_log_event constructor bounded
the length the master sent. queue_event() handed that length to
event_checksum_test(), which subtracted the checksum length from it,
and the result wrapped on an event under four bytes. The constructor
computed the log file name length as event_len minus the header
lengths, and on a truncated heartbeat that subtraction wrapped to a
value near 4GB. queue_event() rejected the heartbeat as invalid, so
its error path appended the log file name to the error message using
the wrapped length.

Add the missing bound in both places. queue_event() now rejects an event
shorter than the common header before anything reads that header, and
the slave IO thread stops with an error. The Heartbeat_log_event
constructor now compares event_len against the combined header lengths
before the subtraction. A heartbeat that clears the first check but
still stops inside its headers keeps ident_len at 0 and log_ident at
NULL, so the error path appends nothing. That constructor check also
precedes the read of the extended log position, which previously ran on
a short event before any validation. A short event from the master now
stops the slave IO thread with an error instead of crashing the server.

This PR is organized in two commits. The first is the regression test and
debug injections; the second is the fix and .result file.

Note I used Claude pretty heavily in creating this patch, yet, I'm sure it took
longer for me to use Claude to go back-and-forth with everything it did
wrong, than if I were to have made this from scratch myself..

[knielsen]

Brandon Nesterenko ***@***.***> writes:

A slave can crash when its master sends an event shorter than the event's own header. A truncated heartbeat makes debug builds fail an assertion in Binary_string::realloc_raw(). On 64-bit release builds it stops the slave IO thread with an error message that omits the log file name. If the master also declares an oversized common header in its format description event, the slave allocates and fills nearly 4GB instead. On 32-bit release builds it writes that error message past the end of a stack buffer. An event under four bytes crashes every build, because the slave checksums roughly 16 EiB and reads far past the end of the packet. Add the missing bound in both places. queue_event() now rejects an event shorter than the common header before anything reads that header, and the slave IO thread stops with an error. The Heartbeat_log_event constructor now compares event_len against the combined header lengths before the subtraction. A heartbeat that clears the first check but still stops inside its headers keeps ident_len at 0 and log_ident at NULL, so the error path appends nothing. That constructor check also precedes the read of the extended log position, which previously ran on a short event before any validation. A short event from the master now stops the slave IO thread with an error instead of crashing the server.

This looks fine, thanks Brandon. I'd just add an unlikely() to this error check (I noticed you put the unlikely() on the similar error check in another place):

+ if (event_len <= all_headers_len) + return; This PR is organized in two commits. The first is the regression test and debug injections; the second is the fix and .result file.

I've seen this pattern used, and I don't mind it as such (ie. it's very good to actually check that the test case fails before the patch). Just want to remark that it breaks bisecting for test case failures. Probably bisecting the full test suite will be impossible anyway with our persistent problems with sporadic test failures. - Kristian.

[knielsen]

Thanks for the quick review!

This looks fine, thanks Brandon. I'd just add an unlikely() to this error check (I noticed you put the unlikely() on the similar error check in another place):

Right, thanks for the reminder

I've seen this pattern used, and I don't mind it as such (ie. it's very

good

to actually check that the test case fails before the patch). Just want to remark that it breaks bisecting for test case failures. Probably bisecting the full test suite will be impossible anyway with our persistent problems with sporadic test failures.

I only use this pattern during the review, so the reviewer can easily debug through and understand the failure. I squash them together before pushing to any main branch. Brandon

…

On Thu, Aug 6, 2026 at 2:29 PM Kristian Nielsen via developers < ***@***.***> wrote: Brandon Nesterenko ***@***.***> writes: > A slave can crash when its master sends an event shorter than the > event's own header. A truncated heartbeat makes debug builds fail an > assertion in Binary_string::realloc_raw(). On 64-bit release builds it > stops the slave IO thread with an error message that omits the log > file name. If the master also declares an oversized common header in > its format description event, the slave allocates and fills nearly 4GB > instead. On 32-bit release builds it writes that error message past > the end of a stack buffer. An event under four bytes crashes every > build, because the slave checksums roughly 16 EiB and reads far past > the end of the packet. > Add the missing bound in both places. queue_event() now rejects an event > shorter than the common header before anything reads that header, and > the slave IO thread stops with an error. The Heartbeat_log_event > constructor now compares event_len against the combined header lengths > before the subtraction. A heartbeat that clears the first check but > still stops inside its headers keeps ident_len at 0 and log_ident at > NULL, so the error path appends nothing. That constructor check also > precedes the read of the extended log position, which previously ran on > a short event before any validation. A short event from the master now > stops the slave IO thread with an error instead of crashing the server. This looks fine, thanks Brandon. I'd just add an unlikely() to this error check (I noticed you put the unlikely() on the similar error check in another place): > + if (event_len <= all_headers_len) > + return; > This PR is organized in two commits. The first is the regression test and > debug injections; the second is the fix and .result file. I've seen this pattern used, and I don't mind it as such (ie. it's very good to actually check that the test case fails before the patch). Just want to remark that it breaks bisecting for test case failures. Probably bisecting the full test suite will be impossible anyway with our persistent problems with sporadic test failures. - Kristian. _______________________________________________ developers mailing list -- ***@***.*** To unsubscribe send an email to ***@***.***

[bnestere]

I wonder why my above email was posted under @knielsen's name. That's worrying..

[knielsen]

Brandon Nesterenko ***@***.***> writes:

I only use this pattern during the review, so the reviewer can easily debug through and understand the failure. I squash them together before pushing to any main branch.

Ah! That's a nice way to do it actually, I hadn't thought of that, thanks for the explanation! - Kristian.