MDEV-40497 KEY_OP_ADD_PREFIX copies changed_length without checking header_end - #5582
Open
KhaledR57 wants to merge 1 commit into
Open
MDEV-40497 KEY_OP_ADD_PREFIX copies changed_length without checking header_end#5582KhaledR57 wants to merge 1 commit into
KhaledR57 wants to merge 1 commit into
Conversation
…eader_end The KEY_OP_ADD_PREFIX branch of _ma_apply_redo_index() passed changed_length from the redo record to memcpy() without checking it against the end of that record, and bounded the page side with a DBUG_ASSERT that release builds compile out. That assert never bounded changed_length at all: it only related it to insert_length, so a record could satisfy it and still copy past the page. Turn the assert into a runtime check with the four bounds the branch needs. The prefix data must be inside the record, the memcpy must stay inside the page, the bmove_upp destination must stay inside the page, and insert_length must not exceed changed_length. The last one is the relation the assert already tested, without it the gap between the two lengths keeps whatever was on the page, and recovery reports success on an index that is corrupt. The test forges one operand per record with four debug keywords, then crashes the server so recovery has to replay them, and each record trips exactly one of the four bounds. The workload splits and then merges index pages, this record comes from the underflow path, so plain inserts never produce one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
KEY_OP_ADD_PREFIXbranch of_ma_apply_redo_index()passed changed_length from the redo record tomemcpy()without checking it against the end of that record, and bounded the page side with aDBUG_ASSERTthat release builds compile out. That assert never boundedchanged_lengthat all, it only related it toinsert_length, so a record could satisfy it and still copy past the page.Turn the assert into a runtime check with the four bounds the branch needs. The prefix data must be inside the record, the
memcpymust stay inside the page, thebmove_uppdestination must stay inside the page, andinsert_lengthmust not exceedchanged_length. The last one is the relation the assert already tested, without it the gap between the two lengths keeps whatever was on the page, and recovery reports success on an index that is corrupt.The test forges one operand per record with four debug keywords, then crashes the server so recovery has to replay them, and each record trips exactly one of the four bounds. The workload splits and then merges index pages, this record comes from the underflow path, so plain inserts never produce one.