Fix CodeQL warning-severity alerts: dead checks, boxed locals and three null dereferences

[vharseko]

Tier 4 of the warning-severity CodeQL triage, covering the remaining sixteen rules (Tier 1 is #789, Tier 2 is #790, Tier 3 is #791). Of the 121 alerts, 63 are fixed here; the other 58 are left as they are, with the reason for each given below.

Real defects, and how reachable each one is

Location	Defect	Reachable?
PurgeConflictsHistoricalTask.runTask()	purgeTaskMaxDurationInSec is Integer.decoded from a task attribute with no upper bound, and * 1000 overflowed int. A max duration of 30 days produced a deadline about 20 days in the past, so purgeConflictsHistorical aborted on the very first entry with ADMIN_LIMIT_EXCEEDED — which runTask() maps to COMPLETED_SUCCESSFULLY, leaving only the purge-completed-in-time attribute at false. Widened to long.	Yes — any value above 2 147 483 seconds, set through an ordinary task attribute
BuildVersion.compareTo():194	rev == version.rev compared String references — rev is a String, as the rev.compareTo(...) two lines below shows — so two builds carrying the same revision from different sources never compared equal. The nested block is now a single Integer.signum(rev.compareTo(...)).	Yes
ImportTask:479	runTask() does not re-validate the task and silently ignores an include branch which resolves to no backend, so backend could still be null and was dereferenced immediately. Guarded with the existing ERR_LDIFIMPORT_NO_BACKENDS_FOR_ID.	Yes — a backend removed between scheduling and execution
ModifyDNOperationBasis.getNewDN():535	The parentDN == null branch set an UNWILLING_TO_PERFORM result and then fell through to parentDN.child(...), throwing NullPointerException. It now returns null once the error is recorded, remembers that the computation has run so the error is not appended a second time, and leaves the INVALID_DN_SYNTAX diagnosis of an unparseable entry DN in place rather than overwriting it. The isRootDN() path is unchanged, since a child DN can still be built there.	The NPE is real, but there is no production caller in tree — getNewDN() is reached only from tests
RootContainer:232	highestID.longValue() was dereferenced although highestID stays null when there is no base DN to open. Entry numbering now starts at 1 in that case.	No — base-dn is mandatory="true" in BackendConfiguration.xml and the sole caller passes config.getBaseDN(), so this is hardening rather than a fix

AttributeFilter (java/iterable-wraps-iterator) kept its iteration state in the Iterable rather than in the iterator, so the Iterable could be traversed only once — a second traversal, including one following its own toString(), yielded nothing. Every in-tree caller traverses exactly once, which makes this a latent violation of the Iterable contract on a public API rather than an active failure. The state now belongs to the iterator, and a test covers both traversals.

The dereferenced-value-may-be-null alert on ImportTask:331 is not fixed: initializeTask() already rejects a task carrying neither a backend ID nor an include branch, and its loop throws for every branch which resolves to no backend, so backend cannot be null there. It is counted below with the other alerts left in place.

Dead checks removed — 19 alerts

Each was verified in place; every removal preserves behaviour.

Location	Why the test was dead
ASCIICharProp ×2	c >= 'a' inside a branch guarded by c >= 'a' && c <= 'z'
StringPrepProfile	the enclosing (b & 0x7F) != b test has already skipped every negative byte
AVA	the NUL comparison sits in the else branch of a c < ' ' test
PasswordPolicyResponseControl	a negative test on a value masked with 0x7F
InetAddressValidator	index > octets.length - 1 under a loop bound of index < octets.length
SchemaUtils ×3	length is already 1, and the first character was validated by the enclosing isAlpha(c) — the check is duplicated, not lost
UTCTimeSyntaxImpl	length < 11 is already rejected at the top of the method
TemplateTag	an array length can never be negative (also closes java/test-for-negative-container-size)
StaticUtils	the second comparison inside firstByte != secondByte is redundant
useless-null-check ×6	NameForm, GenericDialog, BindRule, LDIFBackend, DsMIBImpl ×2 — null checks on fields initialised to a new collection, or on a freshly constructed object

Mechanical fixes — 39 alerts

• 15 boxed locals replaced by primitives (java/non-null-boxed-variable).
• 8 getClass().getResource… calls replaced by the declaring class literal (java/unsafe-get-resource), so the lookup no longer depends on the runtime subclass. The only documented extension point among them is Rest2LdapHttpApplication (two of the eight): a subclass loaded by the same web application resolves identically, but one shipping its own config.json in a separate WAR or bundle would no longer find it. The remaining six are in a final class, two Mojos, a servlet and an internal logger.
• 7 int multiplications widened to long (java/integer-multiplication-cast-to-long). One is the PurgeConflictsHistoricalTask defect above; of the rest only StatusCli's refresh period is also unbounded user input, and the remaining five need values no deployment produces.
• 3 non-short-circuit operators rewritten into explicit locals plus &&/||, with a comment on why both sides must be evaluated — StaticUtils.recursiveDelete and CronExecutorService.cancel/awaitTermination deliberately used &/|, and this keeps that intent while removing the alert.
• 2 getFormatter() calls in JDKLogging qualified with super. (java/subtle-inherited-call). OpenDJHandler is a static nested class, so the unqualified calls already bound to Handler's formatter and not to the enclosing JDKLogging.getFormatter(); the qualification only makes that unambiguous.
• 2 per-call SecureRandom instances in the salted SHA schemes replaced by a shared one (java/random-used-once). The third alert of this rule, in Sha2Crypt, is fixed by Fix CodeQL warning-severity alerts: weak salt PRNG, unsafe DCL, thread-unsafe date formats #789.
• StatusCli refresh loop now sleeps with Thread.sleep and stops when it is interrupted (java/constant-loop-condition). Adding an interrupt check to the loop condition alone would not have worked: StaticUtils.sleep discards the InterruptedException without restoring the flag, so the check could never have fired.
• Dead assignment removed from replace.rb (rb/useless-assignment-to-local).

Left as they are — 58 alerts

Rule	Count	Why
java/dereferenced-value-may-be-null	39	All 43 were reviewed individually rather than dismissed as a group. Three are fixed above — two of them reachable, RootContainer as hardening — one more is already fixed by #789 in AsynchronousTextWriter, and the rest — including ImportTask:331 — are inferred from a guard on a different path or are already excluded by a precondition. Resolving them would mean either adding redundant null checks (making the code worse) or removing guards that may be needed.
java/inconsistent-compareto-and-equals	12	Adding equals/hashCode would change collection semantics for classes that currently rely on identity in HashMap/HashSet. Task was checked as a representative: its compareTo falls back to comparing task IDs, so it returns 0 only for the same task and the TreeSet<Task> usage is already correct.
java/constant-comparison	6	Two are in DoubleMetaphone, a line-by-line port of the Double Metaphone algorithm where simplifying the conditions risks changing phonetic matching results. Four are retry-loop conditions (for (int i = 0; i < 2; ++i), while (stopTries > 0)) that are trivially true on first evaluation; restructuring a retry loop for a cosmetic alert is not worth it.
java/iterator-remove-failure	1	The delegating remove() correctly propagates UnsupportedOperationException.

These are better closed as dismissals in Code scanning with a reason than by changing code, and have been dismissed there.

Verification

• mvn -o verify (javadoc skipped — the generated DsconfigMessages fails the JDK 26 doclint locally, unrelated to this change) for opendj-config and opendj-core: 8173 + 547 tests pass, 0 failures.
• mvn -o compile across all touched modules: passes.
• Both new tests were checked against the pre-fix code and fail there: AttributeFilterTestCase reports expected:<['objectClass', 'cn', 'sn']> but was:<[]>, BuildVersionTest reports expected [0] but found [1].

Note for reviewers

Files also touched by the earlier PRs: Utilities.java and CommonAudit.java (#789), ReplicationBroker.java (#790), Installer.java and BindRule.java (#791). Utilities.java will conflict — #789 edits base lines 2413-2419 and this PR 2411-2417, inside the same isNumericDate branch, and both add a copyright line at the same position. Suggested order: #789 → #790 → #791 → this one.

[maximthomas]

Solid triage — the per-alert justification made this reviewable, and I independently re-derived all 19 dead-check removals: every one is genuinely unreachable, and none widens what the parsers accept. Verified on the PR head: all 9 touched modules compile, opendj-core passes 8171/8171 tests (the only failure was opendj-dsml-servlet's cargo container failing to bind port 8080 locally — unrelated).

Four things to address, mostly around the classification of the fixes rather than their content.

StatusCli interrupt guard can never fire (medium)

opendj-server-legacy/src/main/java/org/opends/server/tools/status/StatusCli.java

StaticUtils.sleep (opendj-server-legacy/src/main/java/org/opends/server/util/StaticUtils.java:2357) swallows InterruptedException without restoring the flag, and Thread.sleep clears it on throw. So after an interrupt isInterrupted() is false and the loop continues exactly as before — I confirmed this with a probe replicating both bodies: 21 iterations post-interrupt(), isInterrupted() == false.

The alert is masked, not fixed, and the new comment describes behaviour the code doesn't have. Either make it real:

if (timeToSleep > 0)
{
  try { Thread.sleep(timeToSleep); }
  catch (InterruptedException e) { Thread.currentThread().interrupt(); break; }
}

…or revert it and dismiss the alert alongside the other 57 — entirely consistent with how the rest of this PR handles cosmetic loop conditions.

ImportTask:331 guard is unreachable (low)

opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java

initializeTask() already enforces the precondition at line 215:

// Make sure that either the "includeBranchStrings" argument or the "backendID" argument was provided.
if (includeBranchStrings.isEmpty() && backendID == null) { throw … ERR_LDIFIMPORT_MISSING_BACKEND_ARGUMENT … }

So at line 328 either backendID != null (and the getLocalBackendById null case already threw), or includeBranches is non-empty and the loop's else throws ERR_NO_BACKENDS_FOR_BASE for every unresolved branch. backend cannot be null.

The runTask guard (:479) is live — runTask never re-validates and silently ignores unresolvable branches (no else), so it's reachable when a backend disappears between scheduling and execution. Keep that one; drop :331, or keep it and label it defensive. As written it's the redundant null check the PR explicitly declines to add for the 37 dereferenced-value-may-be-null alerts.

Defect classification is over-claimed (low)

Of the five highlighted "real defects":

Site	Actually
BuildVersion.compareTo():194	Real, reachable
ImportTask:479	Real, reachable
ModifyDNOperationBasis.getNewDN():530	Real NPE (two null paths: getEntryDN() returns null on a malformed raw DN, and getParentDNInSuffix returns null for a suffix root) — but no in-tree production caller
ImportTask:331	Unreachable (above)
RootContainer:232	Unreachable — base-dn is mandatory="true" in opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/BackendConfiguration.xml:95, and the sole caller passes config.getBaseDN()

Meanwhile the one clearly user-triggerable defect is filed under "mechanical": purgeTaskMaxDurationInSec in opendj-server-legacy/src/main/java/org/opends/server/tasks/PurgeConflictsHistoricalTask.java is Integer.decoded from a task attribute with no upper bound, so e.g. 3000000 overflowed * 1000 to a deadline ~15 days in the past. That belongs in the defects table; the other six widened multiplications are unreachable with realistic inputs.

Also worth softening the AttributeFilter claim: all four in-tree filteredViewOf sinks traverse exactly once and nothing holds an Iterable<Attribute>, so this is a latent Iterable-contract violation on a public API, not an active failure. toString() alone was never broken (each getAllAttributes() call returns a fresh Iterable). The fix is still right.

getNewDN() null result isn't memoised (low)

opendj-server-legacy/src/main/java/org/opends/server/core/ModifyDNOperationBasis.java

null isn't cached in newDN, so a second call repeats the lookup and appends ERR_MODDN_NO_PARENT again — appendErrorMessage concatenates with a " " separator (opendj-server-legacy/src/main/java/org/opends/server/types/AbstractOperation.java:285-287). Low impact (no in-tree caller), but a newDNComputed flag avoids it.

Related: on the malformed-DN path valueOfRawDN has already set INVALID_DN_SYNTAX, which this block then overwrites with UNWILLING_TO_PERFORM — so "the error set above is the outcome" masks the better diagnosis.

Nits

• Duplicate copyright lines: opendj-core/src/main/java/org/forgerock/opendj/ldap/AVA.java already carries Portions copyright 2021-2026 3A Systems, LLC — the added 2026 line is redundant, and it's inserted above the ForgeRock line, breaking chronological order. opendj-core/src/main/java/org/forgerock/opendj/ldap/schema/SchemaUtils.java (2024) and opendj-server-legacy/src/main/java/org/opends/server/types/Entry.java (2023-2024) should extend their ranges rather than gain a second line. The other 42 files are consistent.
• BuildVersion.compareTo computes the comparison twice: rev.equals(…) then rev.compareTo(…). The whole nested block collapses to return Integer.signum(rev.compareTo(version.rev));. (Confirmed the sibling org.opends.server.util.BuildVersion doesn't compare rev at all — no second instance.)
• JDKLogging super. is cosmetic: verified by probe that the unqualified call already bound to inherited Handler.getFormatter(), never the outer static — OpenDJHandler is private static final, so super. ≡ this.. super.setFormatter(…) was never ambiguous. Renaming the outer static would remove the collision at its source.
• Orphaned comment: in runTask the new guard was inserted after // Find backends with subordinate base DNs…, separating it from the code it describes — move the guard above it. Also ERR_LDIFIMPORT_NO_BACKENDS_FOR_ID's text fits :479 but reads oddly for a "neither was specified" case; a comment saying you're reusing it to avoid new message ordinals would help.
• Rest2LdapHttpApplication is the one unsafe-get-resource change with real semantics: the class is explicitly extensible (public, protected final configDirectory/schema, "Default constructor called by the HTTP Framework"). getClass().getResource resolves against the runtime subclass's loader; the class literal against opendj-rest2ldap's. Same webapp loader → identical; a subclass in a separate WAR/bundle shipping its own config.json → breaks. The other seven are safe (ProductInformation is final, Mojos and CommonAudit internal).
• Test coverage: nothing added for six behaviour-affecting fixes. AttributeFilter has no test referencing it at all, yet this PR changes its observable contract — iterate a filtered view twice and assert both traversals match. BuildVersion.compareTo on an equal-but-distinct rev returned 1 before and 0 now: two lines. readOIDLen also has no test (the removal rests purely on int length = 1, which does hold).
• Merge order: Utilities.java will definitely conflict, not just likely — #789 edits base lines 2413-2419, this PR 2411-2417, overlapping in the same isNumericDate branch, plus both add a 3A header line at the same position. Suggested order #789 → #790 → #791 → this one is sound. No semantic overlap on the SecureRandom work (#789 does Sha2Crypt, this does SaltedSHA1/SaltedSHA512) — same pattern, different files.
• Startup cost: the new static final SecureRandom fields seed at class-init in classes the server loads at startup; on entropy-starved hosts with a blocking implementation that's a small delay. A lazy holder would avoid it. Not a blocker.

[vharseko]

Thanks — all four are addressed in 25625bc, along with the nits.

StatusCli interrupt guard

Made real rather than reverted: the loop now sleeps directly and breaks after restoring the flag. StaticUtils.sleep is left alone — restoring the interrupt there would change every one of its callers, which does not belong in this PR.

if (timeToSleep > 0)
{
  try
  {
    // Not StaticUtils.sleep(): it discards the interruption, which would leave this loop
    // running with no way for the caller to stop it.
    Thread.sleep(timeToSleep);
  }
  catch (InterruptedException e)
  {
    Thread.currentThread().interrupt();
    break;
  }
}

ImportTask:331

Dropped — you are right. initializeTask() rejects the "neither backend ID nor include branch" case at line 214, and the loop throws ERR_NO_BACKENDS_FOR_BASE for every branch which resolves to nothing, so backend cannot be null there. The runTask() guard stays, and has been moved above the // Find backends with subordinate base DNs… comment it had separated from, with a note on why the existing message is reused. That alert now counts among the ones left in place, so the totals become 63 fixed / 58 left.

Classification

The description is rewritten. PurgeConflictsHistoricalTask is the first row of the defects table with the overflow spelled out (a 30-day max duration yields a deadline about 20 days in the past, purgeConflictsHistorical then aborts on the very first entry with ADMIN_LIMIT_EXCEEDED, and runTask() maps that to COMPLETED_SUCCESSFULLY), the table gained a reachability column, ImportTask:331 is gone from it, RootContainer:232 is labelled hardening with the mandatory="true" reference, and the AttributeFilter paragraph now says latent Iterable-contract violation rather than active failure.

getNewDN() memoisation

Added a newDNComputed flag, reset in all three raw-DN setters. Took the related point too: the unparseable-DN path now returns before setResultCode, so INVALID_DN_SYNTAX survives instead of being replaced by UNWILLING_TO_PERFORM.

Nits

Duplicate copyright lines merged (AVA's new line removed as redundant, SchemaUtils → 2024-2026, Entry → 2023-2026); compareTo collapsed to Integer.signum(rev.compareTo(version.rev)); the blank line left behind in readOIDLen removed.

Tests

Both of the ones you asked for are in, and both were checked against the pre-fix code:

• AttributeFilterTestCase — a filtered view iterated twice, and iterated after toString(). Against master: expected:<['objectClass', 'cn', 'sn']> but was:<[]>.
• BuildVersionTest — equal-but-distinct revision strings, plus revision and version-number ordering. Against master: expected [0] but found [1].

opendj-core is now 8173 tests and opendj-config 547, both green.

Left alone deliberately

• Rest2LdapHttpApplication keeps the class literal — no subclass in tree, and the rule exists for this exact call. Your caveat about a subclass in a separate WAR is now written into the description.
• JDKLogging.getFormatter not renamed: that changes a public static API for a cosmetic collision.
• No lazy holder for the SecureRandom fields — new SecureRandom() does not block on construction, only seed generation can, so there is no measurable startup cost to avoid.
• readOIDLen still has no direct test; it is covered indirectly by every schema parse carrying a {len} suffix.

One correction

Of the six other widened multiplications, StatusCli's period * 1000 is also unbounded user input: refreshArg is built with .lowerBound(1) and no upper bound, so status -r 3000000 overflowed to a negative sleep and spun on regenerateDescriptor(). Absurd input, but not unreachable. The remaining five do need values no deployment produces.