From 24c91bd64da42a9bea18ad2403ad45b767077df4 Mon Sep 17 00:00:00 2001 From: sunliqiang Date: Sun, 25 Oct 2026 19:20:52 +0800 Subject: [PATCH 1/3] Validate index node key size in the duplicate scanner IndexDuplicateScanner::find() rebuilds each key from a leaf node with m_keyLength = node.length + node.prefix; memcpy(currentKey + node.prefix, node.data, node.length); where prefix and length are taken straight from the index page while currentKey holds BTR_key_length() bytes. Nothing checked the expanded key against the buffer, so a corrupted leaf node with an inflated prefix (the varint allows up to 16383) made the copy run past the buffer. walk_index() in validation.cpp already guards its own scan this way, the scanner did not. Add IndexNode::keyFits() and raise BUGCHECK(204) in the scanner when the node does not fit the key buffer. Checked with a database whose unique unvalidated index has a leaf node with prefix = 16383: ALTER INDEX ... VALIDATE UNIQUE used to crash the engine reading that page, it now stops with the index inconsistency error, and validating a sane index still reports duplicates as before. The range check is covered by the new IndexNodeTest cases running as part of libEngine14_test. --- src/jrd/btn.h | 7 +++++ src/jrd/btr.cpp | 3 ++ src/jrd/tests/IndexNodeTest.cpp | 56 +++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+) create mode 100644 src/jrd/tests/IndexNodeTest.cpp diff --git a/src/jrd/btn.h b/src/jrd/btn.h index 69741956cfd..5cfaf3a9ef9 100644 --- a/src/jrd/btn.h +++ b/src/jrd/btn.h @@ -77,6 +77,13 @@ struct IndexNode return !memcmp(this->data, data + this->prefix, this->length); } + // Check if the key expanded from this node (prefix + length) fits + // into a buffer of the given size + bool keyFits(USHORT size) const + { + return prefix <= size && length <= size - prefix; + } + void setEndBucket() { this->isEndBucket = true; diff --git a/src/jrd/btr.cpp b/src/jrd/btr.cpp index 43a8cc794a0..c25684c6487 100644 --- a/src/jrd/btr.cpp +++ b/src/jrd/btr.cpp @@ -1082,6 +1082,9 @@ bool IndexDuplicateScanner::find(thread_db* tdbb, RecordBitmap& recs) } } + if (!node.keyFits(m_key.getCount())) + BUGCHECK(204); // msg 204 index inconsistent + m_keyLength = node.length + node.prefix; memcpy(currentKey + node.prefix, node.data, node.length); prevRecno = node.recordNumber.getValue(); diff --git a/src/jrd/tests/IndexNodeTest.cpp b/src/jrd/tests/IndexNodeTest.cpp new file mode 100644 index 00000000000..82aa0245c1b --- /dev/null +++ b/src/jrd/tests/IndexNodeTest.cpp @@ -0,0 +1,56 @@ +#include "firebird.h" +#include "boost/test/unit_test.hpp" +#include "../jrd/ods.h" +#include "../jrd/RecordNumber.h" +#include "../jrd/btn.h" + +using namespace Firebird; +using namespace Jrd; + +BOOST_AUTO_TEST_SUITE(EngineSuite) +BOOST_AUTO_TEST_SUITE(IndexNodeSuite) + + +BOOST_AUTO_TEST_SUITE(KeyFitsTests) + +static IndexNode makeNode(USHORT prefix, USHORT length) +{ + IndexNode node; + node.nodePointer = nullptr; + node.prefix = prefix; + node.length = length; + node.pageNumber = 0; + node.data = nullptr; + node.recordNumber = RecordNumber(1); + node.isEndBucket = false; + node.isEndLevel = false; + return node; +} + +BOOST_AUTO_TEST_CASE(KeyFitsExact) +{ + BOOST_TEST(makeNode(0, 0).keyFits(0)); + BOOST_TEST(makeNode(10, 5).keyFits(15)); // prefix + length == size + BOOST_TEST(makeNode(0, 7).keyFits(7)); +} + +BOOST_AUTO_TEST_CASE(KeyFitsTooLarge) +{ + BOOST_TEST(!makeNode(10, 5).keyFits(14)); // one byte short + BOOST_TEST(!makeNode(3, 0).keyFits(2)); // prefix alone overflows + BOOST_TEST(!makeNode(0xFFFF, 0xFFFF).keyFits(0x1000)); + BOOST_TEST(!makeNode(0, 1).keyFits(0)); +} + +BOOST_AUTO_TEST_CASE(KeyFitsRolloverGuard) +{ + // length near USHORT_MAX must not pass through an overflowing sum + BOOST_TEST(!makeNode(1, 0xFFFF).keyFits(0xFFFF)); + BOOST_TEST(makeNode(1, 0xFFFE).keyFits(0xFFFF)); +} + +BOOST_AUTO_TEST_SUITE_END() // KeyFitsTests + + +BOOST_AUTO_TEST_SUITE_END() // IndexNodeSuite +BOOST_AUTO_TEST_SUITE_END() // EngineSuite From 8db612404164e3b408881888a941e79d7808e7cb Mon Sep 17 00:00:00 2001 From: sunliqiang Date: Sun, 25 Oct 2026 21:19:18 +0800 Subject: [PATCH 2/3] Inline the node key check in the duplicate scanner As suggested in the review: the check does not need a helper method on IndexNode, it is a single comparison at the call site. The bound covers the whole copy (prefix + length), since a node with a small prefix but a large length also runs past the key buffer. The trivial unit test goes away with the helper. --- src/jrd/btn.h | 7 ----- src/jrd/btr.cpp | 2 +- src/jrd/tests/IndexNodeTest.cpp | 56 --------------------------------- 3 files changed, 1 insertion(+), 64 deletions(-) delete mode 100644 src/jrd/tests/IndexNodeTest.cpp diff --git a/src/jrd/btn.h b/src/jrd/btn.h index 5cfaf3a9ef9..69741956cfd 100644 --- a/src/jrd/btn.h +++ b/src/jrd/btn.h @@ -77,13 +77,6 @@ struct IndexNode return !memcmp(this->data, data + this->prefix, this->length); } - // Check if the key expanded from this node (prefix + length) fits - // into a buffer of the given size - bool keyFits(USHORT size) const - { - return prefix <= size && length <= size - prefix; - } - void setEndBucket() { this->isEndBucket = true; diff --git a/src/jrd/btr.cpp b/src/jrd/btr.cpp index c25684c6487..f7c7d2185c5 100644 --- a/src/jrd/btr.cpp +++ b/src/jrd/btr.cpp @@ -1082,7 +1082,7 @@ bool IndexDuplicateScanner::find(thread_db* tdbb, RecordBitmap& recs) } } - if (!node.keyFits(m_key.getCount())) + if (currentKey + node.prefix + node.length > m_key.end()) BUGCHECK(204); // msg 204 index inconsistent m_keyLength = node.length + node.prefix; diff --git a/src/jrd/tests/IndexNodeTest.cpp b/src/jrd/tests/IndexNodeTest.cpp deleted file mode 100644 index 82aa0245c1b..00000000000 --- a/src/jrd/tests/IndexNodeTest.cpp +++ /dev/null @@ -1,56 +0,0 @@ -#include "firebird.h" -#include "boost/test/unit_test.hpp" -#include "../jrd/ods.h" -#include "../jrd/RecordNumber.h" -#include "../jrd/btn.h" - -using namespace Firebird; -using namespace Jrd; - -BOOST_AUTO_TEST_SUITE(EngineSuite) -BOOST_AUTO_TEST_SUITE(IndexNodeSuite) - - -BOOST_AUTO_TEST_SUITE(KeyFitsTests) - -static IndexNode makeNode(USHORT prefix, USHORT length) -{ - IndexNode node; - node.nodePointer = nullptr; - node.prefix = prefix; - node.length = length; - node.pageNumber = 0; - node.data = nullptr; - node.recordNumber = RecordNumber(1); - node.isEndBucket = false; - node.isEndLevel = false; - return node; -} - -BOOST_AUTO_TEST_CASE(KeyFitsExact) -{ - BOOST_TEST(makeNode(0, 0).keyFits(0)); - BOOST_TEST(makeNode(10, 5).keyFits(15)); // prefix + length == size - BOOST_TEST(makeNode(0, 7).keyFits(7)); -} - -BOOST_AUTO_TEST_CASE(KeyFitsTooLarge) -{ - BOOST_TEST(!makeNode(10, 5).keyFits(14)); // one byte short - BOOST_TEST(!makeNode(3, 0).keyFits(2)); // prefix alone overflows - BOOST_TEST(!makeNode(0xFFFF, 0xFFFF).keyFits(0x1000)); - BOOST_TEST(!makeNode(0, 1).keyFits(0)); -} - -BOOST_AUTO_TEST_CASE(KeyFitsRolloverGuard) -{ - // length near USHORT_MAX must not pass through an overflowing sum - BOOST_TEST(!makeNode(1, 0xFFFF).keyFits(0xFFFF)); - BOOST_TEST(makeNode(1, 0xFFFE).keyFits(0xFFFF)); -} - -BOOST_AUTO_TEST_SUITE_END() // KeyFitsTests - - -BOOST_AUTO_TEST_SUITE_END() // IndexNodeSuite -BOOST_AUTO_TEST_SUITE_END() // EngineSuite From 9a34079b97389aa0f7aebd05dc10e48a8bf41adb Mon Sep 17 00:00:00 2001 From: sunliqiang Date: Sun, 25 Oct 2026 21:54:20 +0800 Subject: [PATCH 3/3] Check node keys at readNode and release the window on bugcheck As discussed in the review: reject a node as soon as it is read instead of at the copy, and release the CCH window before raising BUGCHECK so the error path does not leave the page latched. --- src/jrd/btr.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/jrd/btr.cpp b/src/jrd/btr.cpp index f7c7d2185c5..396cb7b94a5 100644 --- a/src/jrd/btr.cpp +++ b/src/jrd/btr.cpp @@ -1051,6 +1051,12 @@ bool IndexDuplicateScanner::find(thread_db* tdbb, RecordBitmap& recs) if (node.isEndBucket || node.isEndLevel) break; + if (currentKey + node.prefix + node.length > m_key.end()) + { + CCH_RELEASE(tdbb, &window); + BUGCHECK(204); // msg 204 index inconsistent + } + if (!node.length && !node.prefix && !descending) // all nulls key continue; @@ -1082,9 +1088,6 @@ bool IndexDuplicateScanner::find(thread_db* tdbb, RecordBitmap& recs) } } - if (currentKey + node.prefix + node.length > m_key.end()) - BUGCHECK(204); // msg 204 index inconsistent - m_keyLength = node.length + node.prefix; memcpy(currentKey + node.prefix, node.data, node.length); prevRecno = node.recordNumber.getValue();