lazy: refuse an instance whose data section failed to index, and three uninitialised reads around it - #484
Merged
starseeker merged 4 commits intoSep 10, 2026
Conversation
added 4 commits
August 21, 2026 19:50
isNilSTEPentity( 0 ) returns false, so `if( !isNilSTEPentity( inst ) )` in
loadInstance() is TRUE for a null pointer, and every branch of the switch above
it can leave one:
* case 0 and default leave `inst` at the value returned by
_instancesLoaded.find(), which is 0 on a cache miss;
* case 1 sets `inst = 0` explicitly on the cancellation path added for batch
loading.
A null therefore reaches the success arm and is inserted into _instancesLoaded,
while _loadedInstanceCount, _materializations, _cacheHighWater and the resident
source-byte accounting are all advanced for an instance that was never
materialized. The lazyRefs construction that follows is harmless - its init()
rejects two null arguments - so this is accounting corruption rather than a
crash.
It does not settle either. releaseBatch() skips the entry with `if( !inst )
continue;` BEFORE the `--_loadedInstanceCount` and the erase from
_batchOwnedInstances, so the phantom can never be evicted: the counter stays
high for the life of the manager and _batchOwnedInstances grows across batches.
cacheStatistics() reports the drift.
Guard the pointer as well as the nil sentinel. A null then takes the existing
else arm, which already emits LAZY_DIAGNOSTIC_ERROR "SDAI instance
materialization failed" - the right diagnostic for what happened.
nextInstance() explicitly initialises every member of the local
namedLazyInstance except `name`, and the assignment it does get is conditional:
if( _file.good() ) i.name = getDelimitedKeyword( ";( /\\" );
Nothing is wrong today. When that guard is false the closing "invalid instance"
branch is reached on the same condition and assigns `i.name = 0` before the
struct is returned, so the uninitialised value is never observed - but that is a
control-flow argument spanning forty lines, not an invariant the code states.
It also stops holding the moment getDelimitedKeyword() can fail softly. It
cannot upstream, because it abort()s on a keyword with no delimiter after it; a
caller that would rather report bad input than terminate the host process has to
make it return 0, and then `i.name` is read while uninitialised.
One line, and the struct is uniformly initialised where it is declared.
nextInstance()'s "invalid instance, so clear everything" branch deletes i.refs but leaves the pointer, while componentTypes two lines below is deleted and nulled, and i.name is assigned 0 right after. The struct is returned by value with the stale pointer still in it. Nothing frees it twice today: the same branch zeroes loc.begin, and both callers - the data section constructor's scan loop and indexScope() - stop on that before looking at anything else. So this is the same kind of uniformity fix as the two commits before it rather than a live bug, and it costs one line to stop being a question.
A data section indexes its instances from its own constructor, through addLazyInstance(), under the section id it WILL be given - and lazyFileReader::initP21() registers the section only afterwards, and only if its scan succeeded. A section that fails part way therefore leaves every instance it had already indexed naming a section that does not exist. typeFromFile() then indexes _dataSections with it unconditionally, and loadInstance() does the same behind an assert() that Release compiles out. On an empty vector that is an access violation. sourceRecord() already makes exactly this test; the other two now make it too and answer 0 with a diagnostic, as they already do for an instance found in no section. The instances stay in the index - totalInstanceCount() still counts them and getInstances() still lists them. What changes is that asking for one of them fails the way every other unloadable instance fails. Reachable from a thirteen-line file, added as test/lazy_broken_section.stp and checked by the lazy_index test: a DATA section holding one instance and then a line that is not one. Without the two changes below it, that test dies with SIGSEGV inside typeFromFile(). Note that the new break in loadInstance() leaves `inst` null, and isNilSTEPentity( 0 ) is false, so the null would be cached as a loaded instance were it not for the guard added in "lazy: do not cache a null instance as a loaded one" earlier in this branch. With the bounds test alone the process survives the two calls and then dies on the way out.
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
2 participants
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.
A
lazyDataSectionReaderindexes its instances from its own constructor, throughaddLazyInstance(), under the section id it will be given — andlazyFileReader::initP21()registers it only afterwards, and only if the scan succeeded. A section that fails part way therefore leaves every instance it had already indexed naming a section that does not exist.lazyInstMgr::typeFromFile()indexes_dataSectionswith that id unconditionally, andloadInstance()does the same behind anassert()that Release compiles out. On an empty vector that is an access violation. Thirteen lines reproduce it — aDATAsection holding one instance followed by a line that is not one — and that file is added here assrc/cllazyfile/test/lazy_broken_section.stpand checked by the existinglazy_indextest, which segfaults insidetypeFromFile()without the fix.sourceRecord()already makes exactly this test. The other two entry points now make it too and answer 0 with a diagnostic, as they already do for an instance found in no section. The instances stay in the index —totalInstanceCount()still counts them andgetInstances()still lists them; what changes is that asking for one of them fails the way every other unloadable instance fails.Three smaller things in the same code, each its own commit:
loadInstance()cached a null as a loaded instance.isNilSTEPentity( 0 )returns false, soif( !isNilSTEPentity( inst ) )is true for a null, and several arms of the switch above it can leave one. Nothing dereferences it —lazyRefs::init()rejects two null arguments — but_loadedInstanceCount,_materializations,_cacheHighWaterand the resident source-byte total all advance for an instance that never materialized, and it never settles:releaseBatch()skips the entry withif( !inst ) continue;before the decrement and the erase, so the phantom cannot be evicted. This one is also what keeps the bounds fix above from simply moving the crash to process exit.namedLazyInstance::nameis the one membernextInstance()does not initialise, and its assignment is behindif( _file.good() ). Nothing observes the uninitialised value today — the closing invalid-instance branch is reached on the same condition and assigns 0 — but that is a control-flow argument spanning forty lines rather than an invariant the code states.i.refswithout nulling it, two lines above adelete i.componentTypes; i.componentTypes = 0;that does both.Only the section-bounds fix is a crash; the three above are stated as hardening in their commit messages, not as live bugs.
The commits are ordered so that the null guard comes first: the new
breakinloadInstance()leavesinstnull, so without it the bounds test moves the failure from the call to process exit rather than removing it.Verified both ways on the same build tree (Release, shared libs, no schemas), rebuilding clean between runs:
ctest -R lazy_indexsegfaults on unmodifieddevelopwith the new fixture and assertions, and passes with the four commits. The new case gives itslazyInstMgrthe test's empty registry, so it does not triploadInstance()'sassert( _mainRegistry )in a Debug build.