[Pyolar]

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 encoding allows up to 16383) made the copy run past the key buffer and crash the engine. walk_index() in validation.cpp already guards its own scan this way; the scanner did not.

This adds IndexNode::keyFits() and raises BUGCHECK(204) in the scanner when the node does not fit the key buffer, mirroring the check done during validation.

Checked with a database whose unique unvalidated index (left over by a failed CREATE UNIQUE INDEX ... CONCURRENTLY) carries a leaf node with prefix = 16383: ALTER INDEX ... VALIDATE UNIQUE used to segfault reading that page, it now stops with the index inconsistency error, and validating a sane index still reports duplicates exactly as before. The range check is covered by the new IndexNodeTest cases running as part of libEngine14_test.

[hvlad]

It is enough to check currentKey + m_keyLength < m_key.end() and no need to introduce never used method and test for 2 + 2 == 4
Well, paranoid can also include check node.length + node.prefix <= MAX_USHORT.

[Pyolar]

Agreed, dropped the helper and the test, the check is inline now. One note on the bound itself: checking only currentKey + node.prefix < m_key.end() still lets a node with a small prefix but a large length run past the buffer (the length only has to fit the source page, which is much larger than the key buffer), so I used currentKey + node.prefix + node.length > m_key.end() to cover the whole copy. Happy to reshape it if you prefer.

[hvlad]

Much better, thanks.

One note on the bound itself: checking only currentKey + node.prefix < m_key.end() still lets a node with a small prefix but a large length run past the buffer (the length only has to fit the source page, which is much larger than the key buffer), so I used currentKey + node.prefix + node.length > m_key.end() to cover the whole copy

Sure, and I already corrected myself in this regards.

Before BUGCHECK is it necessary to CCH_RELEASE(tdbb, &window).
The point to consider - should the check better be done closer to node.readNode() ?

[Pyolar]

Right, the window must be released on this path. I'll also move the check right after node.readNode() so a bad node is rejected before anything looks at its fields.