Skip to content

fix: initialise StreamBase::lock_fd to -1 - #5035

Merged
connortechnology merged 1 commit into
ZoneMinder:masterfrom
connortechnology:fix-streambase-lock-fd
Aug 3, 2026
Merged

fix: initialise StreamBase::lock_fd to -1#5035
connortechnology merged 1 commit into
ZoneMinder:masterfrom
connortechnology:fix-streambase-lock-fd

Conversation

@connortechnology

Copy link
Copy Markdown
Member

Split out of the investigation in #5029. Independent of #5033 and #5034.

StreamBase::lock_fd was initialised to 0, but every other use in the class treats a negative value as "no lock held" — openComms() sets it to -1 when open() or flock() fails, and closeComms() guards on lock_fd >= 0 before closing.

The constructor's 0 passes that guard, so a StreamBase destructed without openComms() having succeeded calls close(0) and closes stdin.

Reaching it takes connkey > 0 plus a runStream() that returns before openComms(), and MonitorStream::runStream() has two such returns: the STREAM_SINGLE branch and the !monitor branch. mode=single URLs still carry a connkey, so both are reachable in normal operation.

Severity, honestly: low today. zms exits shortly afterwards and does little in between. But it is closing a descriptor the class doesn't own, and once fd 0 is free the next open() in the process silently lands on it — the kind of thing that turns into a confusing bug later rather than now.

Adds a regression test with a minimal concrete StreamBase so the base constructor and destructor can run without a monitor, database, or shared memory. It saves and restores fd 0, so if it ever fails it reports one section rather than cascading into unrelated tests in the shared binary.

Testing: verified the test fails with lock_fd(0) restored and passes with the fix. Full suite: 117 test cases, 1788 assertions, all passing (Debug, BUILD_TEST_SUITE=ON). Note zm_font needs to run with tests/ as cwd — it uses relative data/fonts/ paths — which is a pre-existing quirk, not related to this change.

lock_fd was initialised to 0, but every other use in the class treats a
negative value as "no lock held": openComms() sets it to -1 when open() or
flock() fails, and closeComms() guards on `lock_fd >= 0` before closing.

The constructor's 0 passes that guard, so a StreamBase destructed without
openComms() having succeeded calls close(0) and closes stdin. Reaching it only
takes connkey > 0 plus a runStream() that returns before openComms(), and
MonitorStream::runStream() has two such returns: the STREAM_SINGLE branch and
the !monitor branch. mode=single URLs still carry a connkey, so both are
reachable in normal operation.

zms exits shortly afterwards and does little in between, so the practical
impact today is small. It is still closing a descriptor the class does not own,
and once fd 0 is free the next open() in the process silently lands on it.

Add a regression test covering destruction with and without a connkey. It saves
and restores fd 0 so a failure can't cascade into the rest of the suite.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@connortechnology
connortechnology merged commit 3705ee4 into ZoneMinder:master Aug 3, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant