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 created issue -
          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 made changes -
          Field Original Value New Value
          Attachment mdev_4645.patch [ 22411 ]
          serg Sergei Golubchik made changes -
          Fix Version/s 10.0.4 [ 13101 ]
          Fix Version/s 5.5.32 [ 13000 ]
          serg Sergei Golubchik made changes -
          Priority Minor [ 4 ] Major [ 3 ]
          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.
          jeremycole Jeremy Cole made changes -
          Attachment mdev_4645_2.patch [ 22432 ]
          serg Sergei Golubchik made changes -
          Assignee Sergei Golubchik [ serg ]

          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?
          serg Sergei Golubchik made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          serg Sergei Golubchik made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          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 made changes -
          Summary Incorrect reads of frozen binlog events Incorrect reads of frozen binlog events; FDE corrupted in relay log
          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.
          jeremycole Jeremy Cole made changes -
          Attachment mdev_4645_3.patch [ 22602 ]
          serg Sergei Golubchik made changes -
          Fix Version/s 5.5.32 [ 13000 ]
          serg Sergei Golubchik made changes -
          Assignee Sergei Golubchik [ serg ] Oleksandr Byelkin [ sanja ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]

          Committed for review (with some fixes).

          sanja Oleksandr Byelkin added a comment - Committed for review (with some fixes).
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]

          Variant with test suite committed for review.

          sanja Oleksandr Byelkin added a comment - Variant with test suite committed for review.
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]
          sanja Oleksandr Byelkin made changes -
          Status In Progress [ 3 ] Stalled [ 10000 ]
          serg Sergei Golubchik made changes -
          Fix Version/s 10.0.5 [ 13201 ]
          Fix Version/s 10.0.4 [ 13101 ]
          sanja Oleksandr Byelkin made changes -
          Status Stalled [ 10000 ] In Progress [ 3 ]

          pushed to 10.0-base

          sanja Oleksandr Byelkin added a comment - pushed to 10.0-base
          sanja Oleksandr Byelkin made changes -
          Resolution Fixed [ 1 ]
          Status In Progress [ 3 ] Closed [ 6 ]
          serg Sergei Golubchik made changes -
          Workflow defaullt [ 27673 ] MariaDB v2 [ 43712 ]
          ratzpo Rasmus Johansson (Inactive) made changes -
          Workflow MariaDB v2 [ 43712 ] MariaDB v3 [ 62877 ]
          serg Sergei Golubchik made changes -
          Workflow MariaDB v3 [ 62877 ] MariaDB v4 [ 146769 ]

          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.