Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ARaveala wants to merge 5 commits into
base: 10.11
Choose a base branch
from
+713 −5
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a46401a
MDEV-39762: add bounds check in query_event_uncompress and row_log_ev…
ARaveala Jun 24, 2026
3ae07a4
MDEV-39762: fix socket binding and fork error handling in mtr_socket_…
ARaveala Jul 20, 2026
5771553
MDEV-39762: fix LocalAddr to use target_host in mtr_socket_relay.pm
ARaveala Jul 24, 2026
8074eba
MDEV-39762: mask GTID position in test cleanup to fix buildbot failure
ARaveala Jul 24, 2026
ee01f18
MDEV-39762: fix GTID conflict in test cleanup on slave binary log
ARaveala Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions mysql-test/lib/mtr_socket_relay.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package mtr_socket_relay;
use Exporter 'import';
our @EXPORT = qw(setup_relay);

use strict;
use warnings;
use IO::Socket::INET;
use POSIX qw(setsid);

# setup_relay($listen_port, $target_host, $target_port)
#
# Generic two-ended socket setup for a relay or proxy script.
# Binds a listen socket, forks and daemonizes so the caller returns
# to MTR immediately, accepts one inbound connection, opens one
# outbound connection to the target, and returns both sockets.
#
# SIGCHLD is set to IGNORE before forking to prevent zombie processes.
#
# Returns ($listen_sock, $inbound, $outbound).

sub setup_relay {
my ($listen_port, $target_host, $target_port) = @_;

my $listen_sock = IO::Socket::INET->new(
LocalAddr => $target_host,
LocalPort => $listen_port,
Type => SOCK_STREAM,
ReuseAddr => 1,
Listen => 5,
Timeout => 60,
) or die "setup_relay: cannot bind $listen_port: $!";
Comment thread
gkodinov marked this conversation as resolved.

warn "PROXY_DEBUG: listening on $listen_port, target at $target_host:$target_port\n";

local $SIG{CHLD} = 'IGNORE';
close STDOUT;
my $pid = fork;
die "setup_relay: fork failed: $!" unless defined $pid;
exit if $pid;
POSIX::setsid();
open(STDOUT, '>', '/dev/null');

my $inbound = $listen_sock->accept()
or die "setup_relay: accept failed: $!";
$inbound->autoflush(1);
warn "PROXY_DEBUG: inbound connection accepted\n";

my $outbound = IO::Socket::INET->new(
PeerHost => $target_host,
PeerPort => $target_port,
Type => SOCK_STREAM,
Timeout => 10,
) or die "setup_relay: cannot connect to $target_host:$target_port: $!";
$outbound->autoflush(1);
warn "PROXY_DEBUG: outbound connection established\n";

return ($listen_sock, $inbound, $outbound);
}

1;
207 changes: 207 additions & 0 deletions mysql-test/lib/rpl_binlog_mitm.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
#!/usr/bin/perl
# rpl_binlog_mitm.pl -- shared MySQL protocol helpers for replication MITM tests.
#
# Provides packet I/O, net_field_length decoding, and the three phases common
# to all replication MITM proxies: master auth, slave handshake, pre-dump relay.
# Each proxy script use() this file and implements its own event patching.
package rpl_binlog_mitm;
use Exporter 'import';
our @EXPORT = qw(
EVENT_TYPE_OFFSET EVENT_LEN_OFFSET COMMON_HEADER_LEN
ROWS_HEADER_LEN_V1 ROWS_HEADER_LEN_V2
FORMAT_DESCRIPTION_EVENT
WRITE_ROWS_COMPRESSED_EVENT WRITE_ROWS_COMPRESSED_EVENT_V1
BAD_LEN_UINT32
read_packet send_packet
net_field_length_size net_field_length_value
do_master_auth do_slave_handshake relay_pre_dump patch_un_len

);
use strict;
use warnings;

# ---------------------------------------------------------------------------
# Constants (from sql/log_event.h)
# ---------------------------------------------------------------------------
use constant EVENT_TYPE_OFFSET => 4;
use constant EVENT_LEN_OFFSET => 9;
use constant COMMON_HEADER_LEN => 19;
use constant ROWS_HEADER_LEN_V1 => 8;
use constant ROWS_HEADER_LEN_V2 => 10;

use constant FORMAT_DESCRIPTION_EVENT => 15; # 0x0f
use constant WRITE_ROWS_COMPRESSED_EVENT => 169; # 0xa9
use constant WRITE_ROWS_COMPRESSED_EVENT_V1 => 166; # 0xa6

# BAD_LEN_UINT32: a uint32 value exceeding MAX_MAX_ALLOWED_PACKET (1GB).
# Used to trigger integer overflow in length field validation tests.
# Value chosen so ALIGN_SIZE() truncation wraps to a small non-zero value.

use constant BAD_LEN_UINT32 => 0xFFFFFFFC;

# ---------------------------------------------------------------------------
# Packet I/O
# ---------------------------------------------------------------------------
sub read_packet {
my ($sock) = @_;
my $hdr = '';
my $n = read($sock, $hdr, 4);
die "read_packet: short header (got " . ($n // 'undef') . ")"
unless defined($n) && $n == 4;
my ($l0, $l1, $l2, $seq) = unpack('CCCC', $hdr);
my $len = $l0 | ($l1 << 8) | ($l2 << 16);
my $data = '';
if ($len > 0) {
$n = read($sock, $data, $len);
die "read_packet: short body" unless defined($n) && $n == $len;
}
return ($seq, $hdr, $data);
}

sub send_packet {
my ($sock, $hdr, $data) = @_;
print $sock $hdr . $data;
}

# ---------------------------------------------------------------------------
# net_field_length -- mirrors net_field_length() in sql/pack.cc
# NOTE: if the encoding changes, update both helpers below.
# ---------------------------------------------------------------------------
sub net_field_length_size {
my ($first) = @_;
return 1 if $first < 251;
return 3 if $first == 252;
return 4 if $first == 253;
return 9;
}

sub net_field_length_value {
my ($data, $offset) = @_;
my $first = unpack('C', substr($data, $offset, 1));
return $first if $first < 251;
return unpack('v', substr($data, $offset + 1, 2)) if $first == 252;
return unpack('V', substr($data, $offset + 1, 3) . "\x00") if $first == 253;
return 0;
}

# ---------------------------------------------------------------------------
# Phase 1a -- authenticate to the real master as root (no password)
# ---------------------------------------------------------------------------
sub do_master_auth {

@gkodinov gkodinov Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ARaveala Jul 21, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ParadoxV5 Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ARaveala Jul 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ARaveala Jul 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gkodinov Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave this decision to the final reviewer.

my ($master) = @_;
my ($seq, $hdr, $greeting) = read_packet($master);
warn "PROXY_DEBUG: master greeting seq=$seq len=" . length($greeting) . "\n";

my $caps = 0x0001 | 0x0200 | 0x8000;
my $auth = pack('V', $caps) . pack('V', 16777216) . pack('C', 8)
. "\x00" x 23 . "root\x00" . "\x00";

print $master pack('CCC', length($auth) & 0xFF,
(length($auth) >> 8) & 0xFF,
(length($auth) >> 16) & 0xFF)
. pack('C', 1) . $auth;

my ($aseq, $ahdr, $ok) = read_packet($master);
my $first = unpack('C', substr($ok, 0, 1));
warn "PROXY_DEBUG: master auth response=0x" . sprintf('%02x', $first) . "\n";
die "PROXY_DEBUG: master auth failed" if $first == 0xff;

}

# ---------------------------------------------------------------------------
# Phase 1b -- send a fresh greeting to the slave and accept any credentials
# ---------------------------------------------------------------------------
sub do_slave_handshake {
my ($slave) = @_;

my $greeting =
"\x0a" . "10.11.0-MariaDB\x00"
. pack('V', 100)
. "12345678\x00"
. pack('v', 0x0001 | 0x0002 | 0x0004 | 0x0200 | 0x8000)
. "\x08" . pack('v', 2) . pack('v', 0) . pack('C', 21)
. "\x00" x 10
. "123456789012\x00";

my $len = length($greeting);
print $slave pack('CCC', $len & 0xFF, ($len >> 8) & 0xFF, ($len >> 16) & 0xFF)
. pack('C', 0) . $greeting;

my ($slave_seq, $hdr, $auth) = read_packet($slave);
warn "PROXY_DEBUG: slave auth seq=$slave_seq\n";

my $ok = "\x00\x00\x00\x02\x00\x00";
print $slave pack('CCC', length($ok), 0, 0) . pack('C', $slave_seq + 1) . $ok;
warn "PROXY_DEBUG: sent OK to slave\n";

}

# ---------------------------------------------------------------------------
# Phase 2 -- relay all pre-dump packets transparently until COM_BINLOG_DUMP.
# Returns when COM_BINLOG_DUMP (0x12) has been forwarded to master.
# ---------------------------------------------------------------------------
sub relay_pre_dump {
my ($master, $slave) = @_;
warn "PROXY_DEBUG: phase2 - pre-dump relay\n";

while (1) {
my ($seq, $hdr, $cmd_data) = read_packet($slave);
my $cmd = unpack('C', substr($cmd_data, 0, 1));
warn "PROXY_DEBUG: slave cmd=0x" . sprintf('%02x', $cmd) . "\n";

send_packet($master, $hdr, $cmd_data);
last if $cmd == 0x12; # COM_BINLOG_DUMP

# relay master's response (OK/ERR or full result set)
my $field_count;
my $in_rows = 0;
while (1) {
my ($rseq, $rhdr, $resp) = read_packet($master);
send_packet($slave, $rhdr, $resp);
my $first = unpack('C', substr($resp, 0, 1));
if (!defined $field_count) {
last if $first == 0x00 || $first == 0xff;
$field_count = $first;
} elsif (!$in_rows) {
$in_rows = 1 if $first == 0xfe && length($resp) < 9;
} else {
last if $first == 0xfe && length($resp) < 9;
}
}
}
warn "PROXY_DEBUG: COM_BINLOG_DUMP forwarded, entering event stream\n";
}

# ---------------------------------------------------------------------------
# patch_un_len($event_data_ref, $ehdr_ref, $flag_offset)
#
# Patches the un_len field at $flag_offset in a compressed binlog event.
# Updates the event length in the common header and the MySQL packet header.
# Operates on references so changes are reflected in the caller.
# ---------------------------------------------------------------------------
sub patch_un_len {
my ($event_data_ref, $ehdr_ref, $flag_offset) = @_;

my $flag_byte = unpack('C', substr($$event_data_ref, $flag_offset, 1));
my $old_len_bytes = $flag_byte & 0x07;

substr($$event_data_ref, $flag_offset, 1 + $old_len_bytes) =
"\x84" . pack('V', BAD_LEN_UINT32);

my $delta = 4 - $old_len_bytes;
if ($delta) {
my $old_len = unpack('V', substr($$event_data_ref, 1 + EVENT_LEN_OFFSET, 4));
substr($$event_data_ref, 1 + EVENT_LEN_OFFSET, 4) =
pack('V', $old_len + $delta);
}

my $new_len = length($$event_data_ref);
substr($$ehdr_ref, 0, 3) = pack('CCC',
$new_len & 0xFF,
($new_len >> 8) & 0xFF,
($new_len >> 16) & 0xFF);

warn "PROXY_DEBUG: patched un_len to 0xFFFFFFFC at offset $flag_offset\n";
}

1;
42 changes: 42 additions & 0 deletions mysql-test/suite/rpl/r/rpl_binlog_compress_overflow.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
include/master-slave.inc
[connection master]
connection slave;
call mtr.add_suppression("Slave I/O: Relay log write failure: could not queue event from master");
call mtr.add_suppression("Error reading packet from server");
call mtr.add_suppression("Slave I/O thread.*exiting");
call mtr.add_suppression("Uncompressed data size too large");
connection master;
SET GLOBAL binlog_checksum = NONE;
SET GLOBAL log_bin_compress = 1;
SET GLOBAL log_bin_compress_min_len = 10;
CREATE TABLE t1 (a VARCHAR(300));
connection slave;
include/stop_slave.inc
CHANGE MASTER TO MASTER_HOST='127.0.0.1', MASTER_PORT=PROXY_PORT, MASTER_USER='root', MASTER_PASSWORD='', MASTER_LOG_FILE='', MASTER_LOG_POS=4, MASTER_USE_GTID=NO;
START SLAVE IO_THREAD;
connection master;
INSERT INTO t1 VALUES ('test');
connection slave;
include/wait_for_slave_io_error.inc [errno=1595]
Last_IO_Error = 'Relay log write failure: could not queue event from master'
include/assert_grep.inc [Uncompressed data size too large]
connection slave;
STOP SLAVE;
Warnings:
Note 1255 Slave already has been stopped
RESET SLAVE ALL;
Warnings:
Note 4190 RESET SLAVE is implicitly changing the value of 'Using_Gtid' from 'No' to 'Slave_Pos'
RESET MASTER;
CHANGE MASTER TO MASTER_HOST='127.0.0.1', MASTER_PORT=MASTER_PORT, MASTER_USER='root', MASTER_PASSWORD='', MASTER_USE_GTID=SLAVE_POS;
connection master;
connection slave;
SET GLOBAL gtid_slave_pos='MASTER_GTID';
include/start_slave.inc
connection master;
SET GLOBAL log_bin_compress = 0;
SET GLOBAL log_bin_compress_min_len = DEFAULT;
SET GLOBAL binlog_checksum = CRC32;
DROP TABLE t1;
connection slave;
include/rpl_end.inc
40 changes: 40 additions & 0 deletions mysql-test/suite/rpl/r/rpl_binlog_compress_overflow_row.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
include/master-slave.inc
[connection master]
connection slave;
call mtr.add_suppression("Slave I/O: Relay log write failure: could not queue event from master");
call mtr.add_suppression("Error reading packet from server");
call mtr.add_suppression("Slave I/O thread.*exiting");
call mtr.add_suppression("Uncompressed data size too large");
connection master;
SET GLOBAL binlog_checksum = NONE;
SET GLOBAL log_bin_compress = 1;
CREATE TABLE t1 (a VARCHAR(300));
connection slave;
include/stop_slave.inc
CHANGE MASTER TO MASTER_HOST='127.0.0.1', MASTER_PORT=PROXY_PORT, MASTER_USER='root', MASTER_PASSWORD='', MASTER_LOG_FILE='', MASTER_LOG_POS=4, MASTER_USE_GTID=NO;
START SLAVE IO_THREAD;
connection master;
INSERT INTO t1 VALUES (REPEAT('x', 256));
connection slave;
include/wait_for_slave_io_error.inc [errno=1595]
Last_IO_Error = 'Relay log write failure: could not queue event from master'
include/assert_grep.inc [Uncompressed data size too large]
connection slave;
STOP SLAVE;
Warnings:
Note 1255 Slave already has been stopped
RESET SLAVE ALL;
Warnings:
Note 4190 RESET SLAVE is implicitly changing the value of 'Using_Gtid' from 'No' to 'Slave_Pos'
RESET MASTER;
CHANGE MASTER TO MASTER_HOST='127.0.0.1', MASTER_PORT=MASTER_PORT, MASTER_USER='root', MASTER_PASSWORD='', MASTER_USE_GTID=SLAVE_POS;
connection master;
connection slave;
SET GLOBAL gtid_slave_pos='MASTER_GTID';
include/start_slave.inc
connection master;
SET GLOBAL log_bin_compress = 0;
SET GLOBAL binlog_checksum = CRC32;
DROP TABLE t1;
connection slave;
include/rpl_end.inc
Loading