MDEV-39762: add bounds check in query_event_uncompress and row_log_ev…

[ARaveala]

Fixes MDEV-39762.

See commit message for full details.

Related:

• MDEV-39674: similar validation gap in Table_map_log_event parsing (same fix pattern)
• MDEV-39672: related overflow in mariadb-binlog Table_map parsing

Open questions for reviewer:

• return 2 is non-idiomatic — enum return type may be preferable
• constructor paths are fixed but untested — acceptable or separate ticket?
• assert_grep targets ER_TOO_BIG_FOR_UNCOMPRESS; easy to revert to generic
  if preferred (see commit message for details)

Test notes: MITM proxy simulates a hostile peer patching un_len in transit,
the only approach that works on release builds (DBUG_EXECUTE_IF is
debug-only). Proxy infrastructure is reusable; if location or naming
of the shared .pm files needs adjusting, that is a move only.
Maintenance: if QUERY_HEADER_LEN or ROWS_HEADER_LEN_V1 change in
log_event.h, the proxy offset calculations need updating — noted in
each proxy file.

License: I agree to license this contribution under the BSD New 3 Clause license.

[gemini-code-assist]

Code Review

This pull request addresses MDEV-39762 by adding validation to prevent stack overflows and crashes caused by oversized un_len fields in compressed binlog events. It introduces checks in sql/log_event.cc and sql/slave.cc to reject events exceeding MAX_MAX_ALLOWED_PACKET and gracefully stop the replica with an appropriate error. Additionally, a Perl-based MITM proxy test suite is added to verify this behavior. The review feedback highlights several improvement opportunities: replacing magic numbers with named constants/enums for return statuses, explicitly binding the test relay socket to 127.0.0.1 instead of 0.0.0.0 for security, and properly handling potential fork failures in the relay script.

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.

[gemini-code-assist]

Using magic numbers like 2 (and 1) as return values for query_event_uncompress and row_log_event_uncompress makes the code harder to maintain and understand. It would be much more idiomatic and self-documenting to define an enum or named constants for these return statuses (e.g., UNCOMPRESS_OK = 0, UNCOMPRESS_ERR = 1, UNCOMPRESS_TOO_BIG = 2) in sql/log_event.h and use them consistently here and in sql/slave.cc.

[gemini-code-assist]

If fork fails, it returns undef. In Perl, fork and exit; will evaluate to false on failure, meaning the parent process will not exit and will continue executing the child's path, leading to unexpected behavior or hangs. It is safer to explicitly check if fork is defined and handle the error.

    my $pid = fork;
    die "setup_relay: fork failed: $!" unless defined $pid;
    exit if $pid;

[ARaveala]

fix:copied instruction.

[ParadoxV5]

Lecture me: Why do we need to test with full-blown Perl-simulated MitM? I’d just craft the malformed event like the JIRA report and #5270 did.

And then, some general things @gkodinov would say: Please rebase to and target 10.6, and fix your CI failures.

[ParadoxV5]

• return 2 is non-idiomatic — enum return type may be preferable

It’s not that non-idiomatic since some old helper functions do return 0/1/2/-1/etc. and only explain what those IDs mean in code comments.

In this specific case, though, returning 0/ER_BINLOG_UNCOMPRESS_ERROR/ER_TOO_BIG_FOR_UNCOMPRESS might be even more idiomatic.

• assert_grep targets ER_TOO_BIG_FOR_UNCOMPRESS; easy to revert to generic
  if preferred (see commit message for details)

That said, whether a new error is preferable over reüsing the generic ER_BINLOG_UNCOMPRESS_ERROR must first depend on whether ER_BINLOG_UNCOMPRESS_ERROR actually makes sense here.

• constructor paths are fixed but untested — acceptable or separate ticket?

I have not looked carefully, but perhaps they are untested because there are now extraneous guards that prevent the constructor fix from taking effect.
These un_len > MAX_MAX_ALLOWED_PACKETs – is this truly a requirement, or have you added a new limitation that the uncompressed data must not exceed 1GB even if it’s not from a 1GB-capped server?

Test notes: MITM proxy simulates a hostile peer patching un_len in transit, the only approach that works on release builds (DBUG_EXECUTE_IF is debug-only).

This is why every (other) fault injection in the repository has --source include/have_debug.inc near the beginning of the test file.

License: I agree to license this contribution under the BSD New 3 Clause license.

At least good job on coming up with the MAX_MAX_ALLOWED_PACKET cap and the MitM test on your own, first-time contributor.
Because my guess is that an AI would just instruct a vibe coder to stick with DBUG_EXECUTE_IFs.

[gkodinov]

Thank you for your contribution. This is a preliminary review.

[gkodinov]

why not target_host? why localhost?

[ARaveala]

Fixed.

[gkodinov]

Not really a fan of this hard-coding of the protocol. It's something that will need updating as we go forward. I'd rather do DBUG_EXECUTE_IF() style of test and inject some faulty data, as the MDEV suggests (and even provides). But this is something for the final reviewer I guess.

[ARaveala]

Thank you for the review. I misunderstood Brandon's Jira suggestion of "pre-creating such an invalid event", I built a MITM proxy thinking it avoided runtime injection, but, it is still effectively runtime injection. I acknowledge the hardcoded protocol is a valid maintenance concern.

Re-reading the Jira discussion, I believe Brandon's suggestion maps to approach 2 below, which may also address your maintenance concern:

1. DBUG_EXECUTE_IF — minimal complexity, as you suggest, but debug-build-only and adds production code
2. Pre-crafted binary file in std_data/ — capture a real compressed event from a running master, patch the un_len bytes offline, commit the result alongside a generator script that documents every byte modified. No production code changes, works on release builds, no protocol hardcoding.

Could you and Brandon confirm the preferred direction? I will hold all changes until clarified.

On the boundary validation gaps in binlog_get_uncompress_len() and binlog_buf_uncompress(), should these be filed as new tickets similar to MDEV-39672/39674, or addressed here?

[ParadoxV5]

DBUG_EXECUTE_IF — minimal complexity, as you suggest, but debug-build-only and adds production code

Debug-build-only means not production code, does it?
Unless, riddle me this, there’re people running debug builds on prod rather than release builds.

On the test approach, the DBUG_EXECUTE_IF injection was noted in the Jira discussion as unnecessarily bloating the code base, so I avoided it.

I indeed objected that the provided injection is a specialized bloat, but I left the discussion open on whether a more generalized and lean injection is possible.

I misunderstood Brandon's Jira suggestion of "pre-creating such an invalid event", I built a MITM proxy thinking it avoided runtime injection, but, it is still effectively runtime injection.

Or, if DBUG_EXECUTE_IF displeases us, handcrafting a binlog file is technically also an option.

On ER_TOO_BIG_FOR_UNCOMPRESS — suggested in the Jira discussion for both query and row events.

👍

The IO thread guard catches network-based attacks; the constructor guard covers direct relay log injection. Currently untested, happy to revisit if it warrants a separate ticket or tests wanted.

🤔 Does the constructor guard also cover network-based attacks if there is no IO thread guard?

[ARaveala]

Thank you for the clarification on DBUG_EXECUTE_IF, I misunderstood the distinction between source bloat and binary bloat. Investigating the constructor guard code path and the handcrafted binlog file approach before replying further. Will follow up shortly.

[ARaveala]

DBUG_EXECUTE_IF is compiled out in release builds so no binary bloat, the concern was source bloat, which remains regardless of build type.

Network path: hostile master → IO thread → queue_event → uncompress guard → relay log. If the guard fires, the event never reaches the relay log and the constructor is never called.
SQL thread path: relay log file → next_event → read_log_event_no_checksum → constructor → uncompress_buf guard. This path seems to bypass queue_event entirely, no equivalent guard was found between next_event and the constructor, though this warrants further review.

Whether the constructor guard provides meaningful protection on the SQL thread path warrants further review.

Side note — DBUG_ASSERT(event_len <= UINT32_MAX) appears in some cases in read_log_event_no_checksum but not the compressed row event cases. Not sure if intentional.

Confirmation needed:

• Binary file approach or DBUG_EXECUTE_IF? i would like to try the binary file approach .
• Both tests (IO thread + SQL thread paths) in this PR, or separate tickets? If binary file approach succeeds, both could be containable under one ticket
• Boundary fixes for binlog_get_uncompress_len/binlog_buf_uncompress, in scope or separate tickets? Function signature changes I would suggest separate tickets. If binary file approach succeeds, a test case for these should follow a similar pattern, could be containable under one ticket.
• ER_TOO_BIG_FOR_UNCOMPRESS, keep or revert to generic error? Can be added or removed easily, open to suggestions on whether the approach can be improved.

[gkodinov]

I will leave this decision to the final reviewer.

[gkodinov]

binlog_get_uncompress_len() itself can read past the buffer's end. it reads the first byte, then decides how much more it must read: 1-3 more. What if that 2nd read goes past end?

[ARaveala]

Valid concern. Initial investigation suggests a check at the call site using the already available comp_len may address this without changing the function signature. A fix inside the function itself would require a signature change, possibly a separate ticket.
While Waiting direction on the test approach, I will explore the binary file approach in the meantime, if it works, testing this scenario with a truncated event should reuse the same infrastructure. Will follow up with findings.

[gkodinov]

same here: this can also go past end

[ARaveala]

end is already available at this point. The existing if (end <= tmp) check can be extended to if (end <= tmp || end - tmp < 1 + (tmp[0] & 0x07)) to also verify there are enough bytes for the length field. Awaiting direction on test approach, but exploring the binary file approach in the meantime, if it works, testing this with a truncated event should reuse the same infrastructure. Will follow up with findings.

[gkodinov]

fwiw, binlog_buf_uncompress() doesn't exactly check lenlen's boundaries too. It could be bigger than len:

int binlog_buf_uncompress(const uchar *src, uchar *dst, uint32 len,
                          uint32 *newlen)
{
  if ((src[0] & 0x80) == 0)
    return 1;

  uint32 lenlen= src[0] & 0x07;
  uLongf buflen= *newlen;                       // zlib type

  uint32 alg= (src[0] & 0x70) >> 4;
  switch(alg) {
  case 0:
    // zlib
    if (uncompress((Bytef *)dst, &buflen,
      (const Bytef*)src + 1 + lenlen, len - 1 - lenlen) != Z_OK)
      return 1;
    break;
  default:
    //TODO
    //bad algorithm
    return 1;
  }

[ARaveala]

Similar boundary validation concern noted. binlog_buf_uncompress receives ```len`` as a parameter which may allow a check inside the function itself. Awaiting test approach direction but will investigate in the meantime.

[ARaveala]

@ParadoxV5 Thank you for the feedback. On the test approach, the DBUG_EXECUTE_IF injection was noted in the Jira discussion as unnecessarily bloating the code base, so I avoided it. The MITM proxy was my interpretation of "pre-creating the invalid event", I acknowledge this is still effectively runtime injection. Awaiting final reviewer direction before changing approach.

On ER_TOO_BIG_FOR_UNCOMPRESS — suggested in the Jira discussion for both query and row events.

On MAX_MAX_ALLOWED_PACKET — this is the compile-time hard ceiling for max_allowed_packet on MariaDB servers. The same concern about non-MariaDB masters applies equally to the guards in query_event_uncompress and row_log_event_uncompress, not just the constructor. If there is a known setup where a non-MariaDB master could produce un_len > MAX_MAX_ALLOWED_PACKET, I am happy to try test against it.

On the constructor guards, called by the SQL thread via the relay log, separate from the IO thread path. The IO thread guard catches network-based attacks; the constructor guard covers direct relay log injection. Currently untested, happy to revisit if it warrants a separate ticket or tests wanted.

[ARaveala]

CI failures (main.except on aarch64, main.innodb_mrr_cpk on embedded) appear unrelated to this fix. rpl_binlog_compress_overflow tests should be skipped on embedded builds via not_embedded.inc

[gkodinov]

Requesting changes because of the 10.11 rebase.

[gkodinov]

LGTM. Please stand by for the final review.