MDEV-40634 Const MEMORY table's BLOB outlives the lock protecting it - #5490
Open
arcivanov wants to merge 1 commit into
Open
MDEV-40634 Const MEMORY table's BLOB outlives the lock protecting it#5490arcivanov wants to merge 1 commit into
arcivanov wants to merge 1 commit into
Conversation
gkodinov
requested changes
Aug 5, 2026
gkodinov
left a comment
Member
There was a problem hiding this comment.
Thank you for working on this. This is a preliminary review.
For the record: the test fails on embedded buildbot hosts. Please rectify. Otherwise, no further comments.
A single-row table is read once during optimization and its row kept in `record[0]` for the rest of the statement. `JOIN::optimize_stage2()` then releases the lock on every const table, on the premise stated in its own comment: *"It's safe to ignore result code as all tables where opened for read only."* That premise assumes a read leaves a **copy** of the row behind. MEMORY with a blob does not. `hp_read_blobs()` answers the read by pointing `record[0]` at the blob data inside `HP_SHARE` rather than copying it, so from the moment the lock is dropped another connection is free to overwrite, free or recycle those bytes -- and the statement goes on reading them. The result is a const table whose value changes in the middle of the statement using it, and a read of freed memory. Let a caller that keeps reading a row after the unlock ask for such tables to be left alone. `GET_LOCK_SKIP_ZERO_COPY_ROWS` drops them from the lock set `get_lock_data()` builds, exactly as `GET_LOCK_SKIP_SEQUENCES` already does, and the const-table unlock in `JOIN::optimize_stage2()` passes it. The skip has to be opt-in rather than a rule. `mysql_lock_remove()` also reaches `mysql_unlock_some_tables()`, and there the unlock is permanent and must not be skipped. A MEMORY blob const table now stays read-locked for the whole statement and blocks writers, which is the price every non-const MEMORY table already pays. The regression test parks the reader with `GET_LOCK()` rather than with a stored function. A stored function puts the statement into prelocked mode, and the const-table unlock is skipped entirely in that mode, so the code path under test would never run. The gate that parks it is taken with `--disable_ps2_protocol` in force. `--ps-protocol` executes every complete `SELECT` twice and compares the two result sets, and `GET_LOCK()` is recursive, so a doubled acquisition would outlive the single `RELEASE_LOCK()` that opens the gate again.
gkodinov
approved these changes
Aug 5, 2026
gkodinov
left a comment
Member
There was a problem hiding this comment.
LGTM! Thanks. Please stand by for the final review.
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
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.
https://jira.mariadb.org/browse/MDEV-40634
The defect
A single-row table is read once during optimization and its row kept in
record[0]for the rest of the statement.JOIN::optimize_stage2()thenreleases the lock on every const table, on the premise stated in its own
comment: "It's safe to ignore result code as all tables where opened for read
only."
That premise assumes a read leaves a copy of the row behind. MEMORY with a
blob does not.
hp_read_blobs()answers the read by pointingrecord[0]at theblob data inside
HP_SHARErather than copying it, so from the moment the lockis dropped another connection is free to overwrite, free or recycle those bytes
— and the statement goes on reading them.
Observed on
06cfb8d0c36(this PR's base), withLENGTH()still reporting 4000throughout because the length lives in
record[0]and only the data moved:UPDATEthenINSERT(blocks recycled)zzzz…— another connection's dataDELETE FROM t1thenINSERT(blocks freed)CHECK TABLEreports OK in both cases: the table is undamaged, the corruptionis entirely in the reader's row.
Pre-existing in the shipped HEAP-blob feature, not introduced by any of the
open fix branches.
The fix
GET_LOCK_SKIP_ZERO_COPY_ROWSdrops such tables from the lock setget_lock_data()builds, exactly asGET_LOCK_SKIP_SEQUENCESalready does, andthe const-table unlock passes it.
The skip is opt-in rather than a rule because
mysql_lock_remove()also reachesmysql_unlock_some_tables(), and there the unlock is permanent and must not beskipped.
Cost: a MEMORY blob const table now stays read-locked for the whole statement
and blocks writers, which is the price every non-const MEMORY table already
pays.
Only the early-unlock path is affected
The three functions that release a lock before the end of a statement were
audited; this is the only unsafe caller.
mysql_unlock_read_tables()—JOIN::join_free, safe:cleanup(full)runsfirst and requires all cursors closed.
mysql_lock_remove()— every call site immediately follows withha_close()/close_thread_table()/drop_temporary_table(), so norecord[0]survives.Non-transactional temporary tables need no handling:
get_lock_data()alreadydrops them from the lock set, so they can neither be early-unlocked nor raced.
Testing
mysql-test/suite/heap/blob_const_unlock.test, deterministic, ~20 ms, coveringboth ways the memory changes hands.
Unit-test-first verified: against the pre-fix binary the test reproduces both
failure modes, and the only difference between the pre-fix and fixed runs is the
two blob reads.
The reader is parked with
GET_LOCK()rather than with a stored function. Thisis load-bearing: a stored function puts the statement into prelocked mode
(
DML_prelocking_strategy::handle_routine()setsneed_prelockingfor anyfunction, even one that touches no tables), and the const-table unlock is
guarded by
!thd->locked_tables_mode, so a function-based test passesvacuously against the broken build.
heap.blob_const_unlock— 20/20 repeatsmain,heapsweep — 1426/1426, zero retries