[sec/first] Address security review findings across Chronicle-Core

[peter-lawrey]

This PR consolidates the Chronicle-Core results from the first deep dive security review wave.

It does two things in parallel:

1. hardens a small number of concrete low-level paths that were relying on weaker or more implicit behaviour
2. converts a broad set of raw-memory, reflection, startup, logging, interrupt, and lifecycle review sites into explicit contracts that the aegis/security toolchain can understand

The largest part of the change is in UnsafeMemory, where caller-owned bounds obligations are now declared explicitly, range assertions are centralised, and many off-heap seams are annotated or reviewed rather than left implicit. The most substantive non-memory hardening is in IOTools.deleteDirWithFiles(...), which is now symlink-safe and no longer risks deleting a plain-file root.

A large proportion of the diff is review metadata, but it is important metadata: in this ruleset, many bare suppressions are not “make the warning disappear” escapes. They are delegate-to-caller contracts.

Important suppression semantics

A key detail for reviewing this PR: many of the bare, non-:silent @SuppressWarnings(...) entries in Chronicle-Core are caller-delegating, not silent waivers.

Per the current SecuritySuppressionTracker behaviour, a bare suppression on these rules means the callee is allowed to remain a reviewed boundary, but the caller must still prove that the use is appropriate. In other words, these suppressions are part of the contract model, not a request to ignore the issue.

The caller-delegating CS rules currently used this way are:

• CSRawAddressAccess
• CSCallerCheckedBounds
• CSClassForNameInput
• CSSetAccessibleEscalation
• CSReflectiveFieldLookup
• CSReflectiveMethodLookup
• CSReflectiveConstructorLookup

That distinction matters for this PR because many reviewed sites are intentionally being converted from implicit behaviour into explicit “caller owns the proof” boundaries.

Two important exclusions:

• :silent suppressions are true silence escapes and do not delegate to the caller
• @CallerCheckedCopyBounds / @CallerCheckedRangeBounds also delegate CSCallerCheckedBounds, but they do so as annotations, not as @SuppressWarnings(...)

So when this PR adds or retains a bare reviewed suppression, the intent is generally:

This boundary is known and reviewed, but its safety or admissibility must still be justified by the caller or enclosing control flow

Why

The aim is to encode the intended contracts clearly enough that future review tooling can distinguish between:

• genuine caller-owned bounds seams and accidental unchecked access
• intentional reflective/bootstrap boundaries and unreviewed escalation
• deliberate best-effort startup/cleanup behaviour and overly broad exception handling
• legitimate direct clock usage and accidental time-provider indirection

Where the review exposed real hardening opportunities, the PR also fixes them. The main concrete fixes are:

• explicit caller-discharged bounds contracts for raw/off-heap memory helpers
• symlink-safe recursive deletion
• improved interrupt handling and lifecycle containment at cleanup/release boundaries
• a small concurrency fix around reference-count/lifecycle state visibility

Key changes

1. Explicit caller-owned bounds contracts for raw-memory seams

This PR introduces new contract types and helpers for low-level memory APIs:

• @CallerCheckedCopyBounds
• @CallerCheckedRangeBounds
• MemoryGuards
• MemoryAegis

These are then applied across UnsafeMemory and related off-heap helpers so that low-level methods can explicitly state when bounds are intentionally discharged by the caller rather than re-checked inside the callee.

This is the central structural change in the PR. It makes the reviewed contract visible, rather than relying on convention or broad suppressions.

In practice, this means:

• copy/range-oriented APIs now declare the address/offset/length tuple they rely on
• assertions are routed through shared helpers with precise failure messages
• many parameters are tightened with @NonNegative / @Positive
• security tooling can treat these methods as reviewed delegate-to-caller boundaries rather than generic unsafe memory access

2. Bare reviewed suppressions now carry contractual meaning

A large part of the diff adds or retains reviewed suppressions with an explicit comment explaining why the boundary exists.

For the caller-delegating rules, these suppressions should be read as part of the API contract, not as “ignore this permanently”. Examples include:

• raw pointer access at tiny off-heap seams
• reflective field/method/constructor lookup that is intentionally centralised
• Class.forName(...) and loadClass(...) entry points on variable names
• controlled setAccessible(...) escalation sites

The important review lens is therefore not merely “is this suppression justified locally?”, but also “is it reasonable to leave the proof obligation with the caller here?”

3. Shared assertion helpers for debug-time memory validation

MemoryAegis centralises assertion-time checks for:

• native address ranges
• object-relative offsets
• object-or-address dual-mode Unsafe access
• byte/char/string slices

This keeps Chronicle’s existing assert SKIP_ASSERTIONS || ... pattern, so the hot-path model stays the same in normal assertions-disabled execution, while debug/test runs get clearer range failures and more consistent validation.

This is especially important in UnsafeMemory, where the old checks were inconsistent across methods and often only asserted non-zero or non-negative inputs rather than validating the full range.

4. Symlink-safe directory deletion and safer file/path handling

IOTools.deleteDirWithFiles(...) has been reworked from a listFiles()/Stream implementation to a Files.walkFileTree(...) implementation with explicit no-follow-link behaviour.

That changes the safety model materially:

• symlinked child directories are not traversed
• symlink placeholders are deleted as links
• plain-file roots are no longer deleted “as if” they were directories
• deletion stays constrained to the intended tree
• concurrent disappearance of files/directories is treated as a degraded but acceptable outcome

Related IO tightening includes:

• Jvm.loadSystemProperties(...) now using Files.newInputStream(...)
• IOTools.urlFor(...) resolving real paths before turning them into file URLs
• IOTools.open(URL) closing the base stream if GZIP wrapping fails
• IOTools.readFile(...) reliably closing the opened stream

5. Interrupt, cleanup, and lifecycle behaviour tightened

A number of cleanup and lifecycle paths were adjusted to preserve or restore interrupt status rather than accidentally consume it.

Notable examples include:

• CloseableUtils
• BackgroundResourceReleaser
• ThreadLocalisedExceptionHandler
• the socket-close locale probing logic in IOTools / IOToolsTest

There are also a few terminal cleanup boundaries that now catch Throwable rather than Exception so cleanup can continue draining remaining work rather than aborting mid-release. These are intentionally limited to last-resort cleanup/release/logging boundaries.

In addition, AbstractCloseableReferenceCounted.initReleased is now volatile, which closes a visibility race between release/close state transitions.

6. Reflection/bootstrap boundaries made explicit rather than implicit

A large part of the diff turns previously implicit reflective/bootstrap behaviour into explicit, reviewed boundaries. That includes:

• Class.forName(...) sites
• reflective field/method/constructor lookup/invocation
• setAccessible(...)
• proxy creation
• service loading
• raw field-offset discovery
• startup hook loading
• last-resort System.err / printStackTrace() fallbacks before logging is fully available

This does not substantially expand the use of reflection. It mostly documents and centralises already-existing reflective behaviour so that future scans can distinguish intentional internals from accidental escalation.

7. Narrower exception handling and cleaner fallback behaviour

Several places that were catching broad Exception or Throwable have been narrowed to the exceptions actually expected:

• IO paths now prefer IOException
• reflection probing often prefers NoSuchMethodException, IllegalAccessException, ClassNotFoundException, etc.
• a few “best effort” metadata/bootstrap lookups now catch RuntimeException instead of all checked exceptions

At the same time, truly terminal containment points remain broad where that is the correct behaviour, for example during cleanup, release draining, chained exception logging, or startup hook execution where failure should be reported but must not stop the rest of the system from continuing.

8. Direct clock use versus provider indirection clarified

Some sites were moved to SystemTimeProvider, while others now include explicit comments stating they must continue using System.nanoTime() or System.currentTimeMillis() directly.

This is mainly about making intent obvious:

• bootstrap-sensitive and provider-implementation code keeps direct system-clock access
• general utility sites that should respect the provider abstraction are routed through SystemTimeProvider
• reviewed direct-clock sites now say explicitly why indirection would be wrong there

9. Targeted test coverage added for the new safety boundaries

The PR adds focused tests for the new helpers and for the changed deletion semantics:

• MemoryAegisTest
• MemoryGuardsTest
• new IOToolsTest cases covering symlinked child directories and plain-file roots

This is important because much of the PR is contract-expression rather than feature work. The tests verify the new safety model rather than just the absence of warnings.

Impact

The practical impact of this PR is:

• raw/off-heap seams can now declare caller-owned bounds obligations explicitly
• bare reviewed suppressions on caller-delegating rules are now easier to read as contractual boundaries rather than as silent waivers
• downstream security scanning should produce fewer false positives and more meaningful reviewed findings
• recursive deletion will no longer traverse symlinked child directories
• recursive deletion will no longer delete a plain-file root by mistake
• interrupt status is preserved more reliably in cleanup/test-support code
• bootstrap, reflection, warning, and lifecycle edges are more precisely documented for the security toolchain

Critical review

This is a useful and worthwhile review pass, but there are a few places reviewers should scrutinise carefully.

1. UnsafeMemory is the load-bearing part of the PR

Most of the UnsafeMemory edits are assertion helpers, annotations, and contract metadata rather than runtime logic changes, but the surface area is huge, and this is one of the highest-risk files in Chronicle-Core.

The most important checks are:

• every @CallerCheckedCopyBounds / @CallerCheckedRangeBounds parameter name matches the real method signature exactly
• the new MemoryAegis assertions still allow all valid existing edge cases
• no important call site was previously relying on ambiguous behaviour that is now rejected more explicitly
• the new range arithmetic in array/object copy methods behaves correctly at overflow boundaries

2. Reviewed suppressions on delegating rules still require caller scrutiny

This PR contains many reviewed suppressions, but many of them are not terminal justifications. For the seven caller-delegating rules listed above, the right review question is not only “is this local suppression reasonable?” but also:

• who is expected to prove the safety of this call?
• is that proof actually available at the caller?
• is the boundary narrow enough that delegating the proof is still safe and understandable?

This matters particularly for:

• CSRawAddressAccess
• CSCallerCheckedBounds
• CSClassForNameInput
• CSSetAccessibleEscalation
• reflective lookup sites that are centralised for later use elsewhere

3. IOTools.deleteDirWithFiles(...) is a real semantic change

This is the strongest concrete hardening in the PR, and it looks directionally correct.

It is also a meaningful behaviour change:

• symlink roots are deleted as links
• symlinked descendants are treated as leaf entries
• plain-file roots now return false
• canonical/real-path resolution is now part of the deletion model

That is safer, but it deserves careful review for edge cases around depth handling, broken links, and platform-specific path behaviour.

4. Some hardening is documentation-only rather than runtime-enforced

The clearest example is UnsafeMemory.freeMemory(...), which now explicitly documents that it is not double-free safe, but still does not prevent double-free.

That is honest and useful, but it is not behavioural hardening. Any expectation of runtime enforcement there would need a separate change.

5. Cleanup paths now prefer containment over propagation in a few more places

That is probably correct at terminal cleanup/release boundaries, but it slightly shifts the balance away from fail-fast and toward drain-and-log.

That should be reviewed with an eye on whether any tests or operators relied on exceptions propagating from those boundaries rather than being logged.

6. The diff is broader than the behavioural delta

The best review order is:

1. UnsafeMemory
2. MemoryAegis, MemoryGuards, CallerCheckedCopyBounds, CallerCheckedRangeBounds
3. IOTools and IOToolsTest
4. lifecycle/cleanup changes (CloseableUtils, BackgroundResourceReleaser, AbstractCloseableReferenceCounted, UnsafeCloseable)
5. the remaining reflection/bootstrap/logging suppressions and catch narrowing

That is the fastest path to the PR's true risk profile.

Draft/scope notes

This PR is opened as a draft because the branch represents one broad review-driven sweep and is easier to inspect as a single review wave than as many tiny suppressions-only changes.

[sonarqubecloud]

Quality Gate passed

Issues
14 New issues
0 Accepted issues

Measures
1 Security Hotspot
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud