Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-4645

Incorrect reads of frozen binlog events; FDE corrupted in relay log

Details

    • Bug
    • Status: Closed (View Workflow)
    • Major
    • Resolution: Fixed
    • 10.0.3, 5.5.31
    • 10.0.5
    • None
    • None
    • All

    Description

      Currently several places use description_event->common_header_len instead of LOG_EVENT_MINIMAL_HEADER_LEN when parsing events with "frozen" headers (such as Start_event_v3 and its subclasses such as Format_description_log_event, as well as Rotate_event). This causes events with extra headers (which would otherwise be valid and those headers ignored) to be corrupted due to over-reading or skipping into the data portion of the log events.

      Patch is forthcoming.

      Attachments

        1. mdev_4645_2.patch
          1 kB
        2. mdev_4645_3.patch
          1 kB
        3. mdev_4645.patch
          16 kB

        Activity

          jeremycole Jeremy Cole added a comment -

          The attached patch addresses the following:

          • A fairly major restructuring of the way that mysqlbinlog H/-hexdump produces output. The new structure formats better (more closely to hexdump -C) without alignment errors, and with easier to visually parse fields. Additionally it is more extensible, allowing for downstream users (i.e. me) to parse additional header information with simpler patches.
          • Introduces a method in Log_event::has_minimal_header() in order to simplify logic in other places that need to determine whether a log event should have a fixed minimal header or a common header.
          • Ensure mysqlbinlog H/-hexdump handles headers of variable lengths (as above) correctly.
          • Ensure Start_log_event_v3 (and its subclasses) and Rotate_log_event consistently use LOG_EVENT_MINIMAL_HEADER_LEN as the size of their (frozen) header. Multiple instances incorrectly used description_event->common_header_len instead.
          • Fix incorrect usage of common_header_len (via buf[LOG_EVENT_MINIMAL_HEADER_LEN + ST_COMMON_HEADER_LEN_OFFSET]) instead of LOG_EVENT_MINIMAL_HEADER_LEN in get_checksum_alg for FORMAT_DESCRIPTION_EVENT.
          jeremycole Jeremy Cole added a comment - The attached patch addresses the following: A fairly major restructuring of the way that mysqlbinlog H/ -hexdump produces output. The new structure formats better (more closely to hexdump -C) without alignment errors, and with easier to visually parse fields. Additionally it is more extensible, allowing for downstream users (i.e. me) to parse additional header information with simpler patches. Introduces a method in Log_event::has_minimal_header() in order to simplify logic in other places that need to determine whether a log event should have a fixed minimal header or a common header. Ensure mysqlbinlog H/ -hexdump handles headers of variable lengths (as above) correctly. Ensure Start_log_event_v3 (and its subclasses) and Rotate_log_event consistently use LOG_EVENT_MINIMAL_HEADER_LEN as the size of their (frozen) header. Multiple instances incorrectly used description_event->common_header_len instead. Fix incorrect usage of common_header_len (via buf [LOG_EVENT_MINIMAL_HEADER_LEN + ST_COMMON_HEADER_LEN_OFFSET] ) instead of LOG_EVENT_MINIMAL_HEADER_LEN in get_checksum_alg for FORMAT_DESCRIPTION_EVENT.
          jeremycole Jeremy Cole added a comment -

          Additionally, see mdev_4645_2.patch which should be applied on top of mdev_4645.patch. This fixes a 1-byte buffer overflow in the event printing code which was caught by our address sanitizer testing. Apologies for the error in the original patch.

          jeremycole Jeremy Cole added a comment - Additionally, see mdev_4645_2.patch which should be applied on top of mdev_4645.patch. This fixes a 1-byte buffer overflow in the event printing code which was caught by our address sanitizer testing. Apologies for the error in the original patch.

          Jeremy, under what license do we have these patches? If it's new BSD, could you say that explicitly, please?

          serg Sergei Golubchik added a comment - Jeremy, under what license do we have these patches? If it's new BSD, could you say that explicitly, please?

          And, why would this issue matter? Under what circumstances is common_header_len not equal to LOG_EVENT_MINIMAL_HEADER_LEN?

          serg Sergei Golubchik added a comment - And, why would this issue matter? Under what circumstances is common_header_len not equal to LOG_EVENT_MINIMAL_HEADER_LEN?
          jeremycole Jeremy Cole added a comment -

          Will get you a definitive answer shortly on license.

          Regarding why it matters: For any MySQL/MariaDB deployed in the wild, you're quite right, it doesn't matter, since they are always the same value. So, for those cases this is merely semantics. However, Google has patched MySQL 5.1 to add additional headers, increasing the size of headers (to 31 bytes) for non-frozen binlog events. Correct parsing of the header depends on the size being correct in these instances, and they were found by MariaDB 10 not being able to replicate - at all - from MySQL 5.1.

          So, for us it is quite critical, but for everyone else it's just paint touch-ups on the underside of a cabinet.

          The formatting changes for mysqlbinlog hexdump output are of course useful to anyone, I'd guess. The original state was a mess.

          jeremycole Jeremy Cole added a comment - Will get you a definitive answer shortly on license. Regarding why it matters: For any MySQL/MariaDB deployed in the wild, you're quite right, it doesn't matter, since they are always the same value. So, for those cases this is merely semantics. However, Google has patched MySQL 5.1 to add additional headers, increasing the size of headers (to 31 bytes) for non-frozen binlog events. Correct parsing of the header depends on the size being correct in these instances, and they were found by MariaDB 10 not being able to replicate - at all - from MySQL 5.1. So, for us it is quite critical, but for everyone else it's just paint touch-ups on the underside of a cabinet. The formatting changes for mysqlbinlog hexdump output are of course useful to anyone, I'd guess. The original state was a mess.
          jeremycole Jeremy Cole added a comment - - edited

          serg: The patches attached to this ticket (mdev_4645.patch, mdev_4645_2.patch, mdev_4645_3.patch) are provided under the "New BSD" License by Google Inc.

          jeremycole Jeremy Cole added a comment - - edited serg : The patches attached to this ticket (mdev_4645.patch, mdev_4645_2.patch, mdev_4645_3.patch) are provided under the "New BSD" License by Google Inc.
          jeremycole Jeremy Cole added a comment -

          Attaching mdev_4645_3.patch which fixes a related issue where the Format_description_log_event event's common_header_len field would be overwritten by the slave's LOG_EVENT_HEADER_LEN value rather than preserving the common_header_len received in the original event.

          jeremycole Jeremy Cole added a comment - Attaching mdev_4645_3.patch which fixes a related issue where the Format_description_log_event event's common_header_len field would be overwritten by the slave's LOG_EVENT_HEADER_LEN value rather than preserving the common_header_len received in the original event.

          Committed for review (with some fixes).

          sanja Oleksandr Byelkin added a comment - Committed for review (with some fixes).

          Variant with test suite committed for review.

          sanja Oleksandr Byelkin added a comment - Variant with test suite committed for review.

          pushed to 10.0-base

          sanja Oleksandr Byelkin added a comment - pushed to 10.0-base

          People

            sanja Oleksandr Byelkin
            jeremycole Jeremy Cole
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Git Integration

                Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.