_altname is a char[BUFSIZ+1] that neither constructor touches, so CurrName()
and the AltNameOf paths can read uninitialised heap until something calls
AltName(). Start it empty.
getline(buf, BUFSIZ, ';') on a stream whose next BUFSIZ bytes contain no
semicolon sets failbit and leaves buf unchanged. The scan loop only tests
eof(), which never arrives once failbit is set, so it never terminates - hand
it any binary file and it hangs instead of reporting that the file is not
STEP. Treat a failed extraction the same as end of file.
STEPnode::_null had no default: STEPnode has no constructor, so a node built
by a subclass that does not assign it starts out with whatever was on the
heap, and is_null() then answers at random. Give STEPnode a constructor that
starts nodes null.
EntityNode::StrToVal() and ::STEPread() additionally have to set it per
outcome - they resolve an entity reference and must report null exactly when
the reference did not resolve. Before this, a node whose reference failed to
resolve could still report itself non-null.
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.
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.
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.
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.
"cmake: declare target link interfaces explicitly" (0b950dd1) made SC_ADDLIB
publish its LINK_LIBRARIES as PUBLIC. That is the right change, but it is not
compatible with the way src/cldai/CMakeLists.txt names its static dependency:
SC_ADDLIB(stepdai-static STATIC SOURCES ${DAI_SRCS}
LINK_LIBRARIES $<JOIN:${_libdeps},-static >-static)
_libdeps holds one entry, steputils, so the generator expression is an elaborate
way of writing steputils-static. While the dependency was linked privately it
expanded once, on the link line of stepdai-static, and was correct. Published as
PUBLIC it lands in INTERFACE_LINK_LIBRARIES and is evaluated again in every
consumer, where it comes back out mangled and reaches the linker verbatim:
LINK : fatal error LNK1104: cannot open file $<JOIN:steputils,-static.obj
p21read_sdai_<schema> is the first consumer to hit it, so with BUILD_STATIC_LIBS=ON
no schema executable links. Reproduced on develop tip - nothing after 0b950dd1
touches either file, so the static build is broken there too.
Building the list with a foreach costs nothing, reads the same, and keeps working
if _libdeps ever grows. The shared branch is untouched; this is inside
if(BUILD_STATIC_LIBS).
2026-08-19 13:16:28 +02:00
13 changed files with 119 additions and 18 deletions
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Clifford Yapp