Repository navigation
Conversation
A slot left in the irt_migrate state by ALTER INDEX ... SET TABLESPACE stores the number of the slot holding the original index in irt_backslot. getIndexSlot() and the irt_migrate branch of modifyIrtRepeat() turned that field into &irt_rpt[backId] with only an fb_assert guarding the range, so a corrupted index root page moved both the reads in getIndexSlot() and the setKill() write in modifyIrtRepeat() up to 65535 * 24 bytes past the page buffer. Add index_root_page::getBackSlot() returning NULL when the stored slot points outside the page and raise BUGCHECK(204) in both call sites instead of using the bad pointer. Checked with a database whose index root page carries a migrated slot with irt_backslot = 0xFFFF: CREATE INDEX used to corrupt engine memory, now it stops with the index inconsistency error while normal DDL and DML are unaffected. Unit tests for the slot range check run as part of libEngine14_test.
getBackSlot() encapsulates the backslot logic and may later be used by callers that did not check the slot state first. Assert the slot is in the irt_migrate state and return NULL for other states as well, so such a call still ends in BUGCHECK(204) in release builds. The definitions move below the irt_* state constants (irt_migrate is declared after index_root_page), matching the layout of the other irt_repeat methods. IndexRootPageTest now builds the migrating slot through the regular state transitions and adds a case for a non-migrating slot.
The non-const overload now delegates to the const one instead of repeating its body.
As discussed in the review: getIndexSlot() now takes the window and releases the index root page before raising BUGCHECK, the check in modifyIrtRepeat() runs before CCH_MARK() with the release in place, and BTR_description() and walk_index() pass their windows through so the error path never leaves the page latched.
|
Fixed, please review |
| bool BTR_description(thread_db* tdbb, Cached::Relation* relation, const index_root_page* root, index_desc* idx, | ||
| MetaId id, USHORT flags) | ||
| bool BTR_description(thread_db* tdbb, Cached::Relation* relation, const index_root_page* root, WIN* window, | ||
| index_desc* idx, MetaId id, USHORT flags) |
There was a problem hiding this comment.
I think it's redundant to pass both root and window, as root == window->win_buffer and thus can be retrieved inside the routine based on window. @hvlad , do you agree?
There was a problem hiding this comment.
I think it's redundant to pass both
rootandwindow
+1
There was a problem hiding this comment.
Sure, it is redundant.
But I don't like what we have for now - for such a small goal too much changes in code.
There should be a better way to reach it, not poisoning too much code around.
There was a problem hiding this comment.
Agreed. Passing WIN* through getIndexSlot() / BTR_description() makes the change spread much further than the actual backslot validation requires.
Would it be better to keep the existing BTR_description() / getIndexSlot() interfaces unchanged and handle the corrupted backslot locally?
My idea is to keep getBackSlot() for validating the irt_migrate state and irt_backslot < irt_count, and on failure use:
CCH_unwind(tdbb, false);
BUGCHECK(204);in the two places that dereference irt_backslot (getIndexSlot() and modifyIrtRepeat()).
For modifyIrtRepeat() the validation would still happen before CCH_MARK().
This should avoid passing WIN* through unrelated callers and reduce the patch back to the actual backslot handling. Would this be a better direction?
There was a problem hiding this comment.
Implemented — the WIN* threading has been removed again, so getIndexSlot() and BTR_description() keep their original interfaces and the patch is now limited to btr.cpp, ods.h and the unit test.
For the corrupted backslot path in getIndexSlot(), the current call paths use the index root page read-latched and not marked, so I use:
CCH_unwind(tdbb, false);
BUGCHECK(204);to release the held cache latches before reporting the index inconsistency.
For modifyIrtRepeat(), I use CCH_RELEASE(tdbb, window) instead. In particular, BTR_reserve_slot() may already have marked the root page before calling modifyIrtRepeat(), while CCH_unwind() refuses to unwind marked buffers and would raise BUGCHECK(268). The backslot validation therefore remains before the local CCH_MARK() and the current window is released explicitly on the error path.
This keeps the validation local to the two places that actually dereference irt_backslot and avoids changing unrelated interfaces and callers.
Please let me know if this direction looks better.
Drop the WIN* threading again: getIndexSlot() and BTR_description() keep their signatures, so the fix stays local to the backslot handling. getIndexSlot() callers hold the index root page read-latched and never marked, so CCH_unwind(tdbb, false) releases the latched pages before BUGCHECK(204) reports the inconsistency. modifyIrtRepeat() runs on paths where BTR_create() has already marked the page and CCH_unwind() refuses to release marked buffers, so it releases its own window with CCH_RELEASE() - the window is already a parameter there. The check in modifyIrtRepeat() stays ahead of CCH_MARK() either way.
A slot left in the
irt_migratestate byALTER INDEX ... SET TABLESPACEstores the number of the slot holding the original index inirt_backslot.getIndexSlot()and theirt_migratebranch ofmodifyIrtRepeat()turned that field into&irt_rpt[backId]with nothing but anfb_assertchecking the range, so on a corrupted index root page the reads ingetIndexSlot()and thesetKill()write inmodifyIrtRepeat()landed up to65535 * 24bytes past the page buffer, andBTR_delete_index()then reused the same out-of-range slot number.This adds
index_root_page::getBackSlot(), which resolves the backing slot and returns NULL when the stored number points outsideirt_rpt. Both call sites raise BUGCHECK(204) instead of following the bad pointer.Checked with a database whose index root page carries a migrated slot with
irt_backslot = 0xFFFF:CREATE INDEXused to corrupt engine memory on that database (ASan reports an invalid write before the crash), it now stops with the index inconsistency error. NormalCREATE INDEX, DML and index activation on a sane database are unaffected. The slot range check is covered by the newIndexRootPageTestcases running as part oflibEngine14_test.