Skip to content

MDEV-40759: Fix --force-read crash on checksum-failing Format Description event #5552

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
prathamesh04 wants to merge 1 commit into
base: main
Choose a base branch
from
+90 −10
Open

MDEV-40759: Fix --force-read crash on checksum-failing Format Description event #5552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 25 additions & 9 deletions client/mysqlbinlog.cc
View file
Open in desktop

@ParadoxV5 ParadoxV5 Aug 15, 2026

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.

This approach is new to me, but I’m not convinced.

A Format Description Event that’s invalid (whether because of a checksum mismatch or some other corruption) is still meant to describe the log’s format.
Both ignoring it and substituting with “a default” FDE would be very wrong (at least for the premise of this check_header() function, in contrast to the dream where the default FDE is already sufficient).

The only case this would not matter is when two FDEs appear back-to-back – this can only come from a handcrafted log, AFAIK.

Original file line number Diff line number Diff line change
Expand Up @@ -3187,38 +3187,54 @@ static Exit_status check_header(IO_CACHE* file,
else if (buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT)
{
/* This is 5.0 */
Format_description_log_event *new_description_event;
Log_event *new_description_event;
my_b_seek(file, tmp_pos); /* seek back to event's start */
if (!(new_description_event= (Format_description_log_event*)
Log_event::read_log_event(file, &read_error,
glob_description_event,
opt_verify_binlog_checksum)))
if (!(new_description_event= Log_event::read_log_event(file, &read_error,
glob_description_event,
opt_verify_binlog_checksum)))
/* EOF can't be hit here normally, so it's a real error */
{
error("Could not read a Format_description_log_event event at "
"offset %llu; this could be a log format error or read error.",
(ulonglong)tmp_pos);
return ERROR_STOP;
}
if (new_description_event->get_type_code() != FORMAT_DESCRIPTION_EVENT)
{
/*
With --force-read, an event whose checksum is invalid is
returned as an Unknown_log_event instead of NULL. Such an event
does not describe the log's format, so report it the way the main
loop would and continue with the default description event below.
*/
DBUG_ASSERT(new_description_event->get_type_code() == UNKNOWN_EVENT);
Exit_status retval= process_event(print_event_info,
new_description_event, tmp_pos,
logname);
if (retval != OK_CONTINUE)
return retval;
break;
}
Format_description_log_event *new_fde=
static_cast<Format_description_log_event *>(new_description_event);
if (opt_base64_output_mode == BASE64_OUTPUT_AUTO)
{
/*
process_event will delete *description_event and set it to
the new one, so we should not do it ourselves in this
case.
*/
DBUG_ASSERT(tmp_pos + new_description_event->data_written ==
DBUG_ASSERT(tmp_pos + new_fde->data_written ==
my_b_tell(file));
Exit_status retval= process_event(print_event_info,
new_description_event, tmp_pos,
Exit_status retval= process_event(print_event_info, new_fde, tmp_pos,
logname);
if (retval != OK_CONTINUE)
return retval;
}
else
{
delete glob_description_event;
glob_description_event= new_description_event;
glob_description_event= new_fde;
}
DBUG_PRINT("info",("Setting description_event"));
}
Expand Down
25 changes: 25 additions & 0 deletions mysql-test/suite/binlog/r/binlog_mysqlbinlog_force_read.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
CREATE TABLE t1 (a INT);
INSERT INTO t1 VALUES (1);
/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=1*/;
/*!40019 SET @@session.max_delayed_threads=0*/;
/*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
DELIMITER /*!*/;
# at 4
# Encrypted event
DELIMITER ;
# End of log file
ROLLBACK /* added by mysqlbinlog */;
/*!50003 SET COMPLETION_TYPE=@OLD_COMPLETION_TYPE*/;
/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=0*/;
/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=1*/;
/*!40019 SET @@session.max_delayed_threads=0*/;
/*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
DELIMITER /*!*/;
# at 4
# Encrypted event
DELIMITER ;
# End of log file
ROLLBACK /* added by mysqlbinlog */;
/*!50003 SET COMPLETION_TYPE=@OLD_COMPLETION_TYPE*/;
/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=0*/;
DROP TABLE t1;
29 changes: 29 additions & 0 deletions mysql-test/suite/binlog/t/binlog_mysqlbinlog_force_read.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# MDEV-40759: mariadb-binlog --force-read --start-position crashes on
# Unknown event
#
# With --force-read, a Format Description event whose checksum is invalid is
# returned as an Unknown_log_event instead of NULL (Log_event::read_log_event).
# check_header()'s prelude wrongly treated such an event as a Format
# Description event, which is undefined behaviour (a vptr error under UBSan)
# and, in --base64-output modes other than AUTO, silently swallowed the event.
#
# The checksum failure is simulated with --debug=d,simulate_checksum_test_failure.
--source include/mysqlbinlog_have_debug.inc

@ParadoxV5 ParadoxV5 Aug 15, 2026

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.

Hm, what’s this? It’s not used in either 10.11 or the base main… 🧐🤔

@ParadoxV5 ParadoxV5 Aug 15, 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.

It came from before the hard-forking and is never used…

Personal opinion: It’s good that it’s not used, because it works by scanning the entire mariadb-binlog --help…
(Unlike have_debug.inc from long ago; though suite.pm still scans the entire mariadbd --help, from which have_debug.inc is now derived.)
There ought to be more efficient ways.

--source include/have_binlog_format_mixed.inc

# Grow the current binlog past the Format Description event so that
# check_header()'s --start-position prelude has to read it.
CREATE TABLE t1 (a INT);
Comment on lines +14 to +16

@ParadoxV5 ParadoxV5 Aug 15, 2026

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.

FYI: A modern “blank” binlog is already past the FDE: There’s the GTID List, a Binlog Checkpoint for XA, maybe (forgot if there is) a Rotate.

Manually growing the log is also fine, to not depend on those additional header events, especially since we do want to test with some manually controlled content below.

INSERT INTO t1 VALUES (1);

--let $MYSQL_DATADIR= `SELECT @@datadir`
--let $binlog_file= query_get_value(SHOW BINLOG STATUS, File, 1)
--let $binlog_start= query_get_value(SHOW BINLOG STATUS, Position, 1)

Comment on lines +17 to +22

@ParadoxV5 ParadoxV5 Aug 15, 2026

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.

In fact, you can smell how wrong it gets from how mariadb-binlog fails if you have some content after start-position:

Suggested change
INSERT INTO t1 VALUES (1);
--let $MYSQL_DATADIR= `SELECT @@datadir`
--let $binlog_file= query_get_value(SHOW BINLOG STATUS, File, 1)
--let $binlog_start= query_get_value(SHOW BINLOG STATUS, Position, 1)
--let $MYSQL_DATADIR= `SELECT @@datadir`
--let $binlog_file= query_get_value(SHOW BINLOG STATUS, File, 1)
--let $binlog_start= query_get_value(SHOW BINLOG STATUS, Position, 1)
INSERT INTO t1 VALUES (1);

In the final test, you can use the DROP below as the “content after start-position” rather than this INSERT.

# Must not crash, and must report the checksum-failing Format Description event.
--exec $MYSQL_BINLOG --verify-binlog-checksum --force-read --debug=d,simulate_checksum_test_failure --start-position=$binlog_start $MYSQL_DATADIR/$binlog_file

# Must not silently drop the event when not in AUTO mode either.
--exec $MYSQL_BINLOG --verify-binlog-checksum --force-read --base64-output=NEVER --debug=d,simulate_checksum_test_failure --start-position=$binlog_start $MYSQL_DATADIR/$binlog_file

DROP TABLE t1;
12 changes: 11 additions & 1 deletion sql/log_event.cc
View file
Open in desktop

@ParadoxV5 ParadoxV5 Aug 15, 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.

Thanks for your interest, but we do merge-ups instead of backports here, so please base your PR on 10.11 (where another fix has made this part of your patch obsolete) rather than main.

Original file line number Diff line number Diff line change
Expand Up @@ -930,11 +930,21 @@ Log_event* Log_event::read_log_event(IO_CACHE* file, int *out_error,
err:
if (unlikely(error))
{
DBUG_ASSERT(!res);
#ifdef MYSQL_CLIENT
/*
read_log_event(char*,...) may return a best-effort Unknown_log_event
together with an error string when reading with --force-read. Such an
event cannot be used (it carries no format information), so free it and
report a generic unknown event instead. The "no result on error"
assertion below only applies to the non-force-read case.
*/
if (force_opt)
{
delete res;
DBUG_RETURN(new Unknown_log_event());
}
#endif
DBUG_ASSERT(!res);

/*
The SQL slave thread will check *out_error to know
Expand Down
Loading