Resolve open CodeQL alerts in the persistit module

[vharseko]

Clears the open CodeQL code-scanning alerts in the persistit module (1 high, 2 warnings, 8 notes).

Framing: this is a static-analysis legibility fix, not a security fix, so the security label has been dropped. The "high" java/improper-validation-of-array-index finding was a false positive — the original bounds check was already correct — so nothing security-relevant changed.

High

• java/improper-validation-of-array-index — Exchange.fetchBufferCopy: the level cache is captured into a local final LevelCache[] levelCache = _levelCache and the bounds check is expressed with Objects.checkIndex, which CodeQL recognizes as an array-index sanitizer. The bound is genuinely reachable — tree depth can legally reach PAGE_TYPE_INDEX_MAX and is unbounded on a corrupted volume — so the original IllegalArgumentException("Tree depth is N") contract (message and exception type) is preserved by rethrowing from the IndexOutOfBoundsException. The thrown exception is now documented in the javadoc.

Warnings

• java/input-resource-leak — CLI.source: the FileReader is now wrapped directly in the BufferedReader pushed onto _sourceStack (also drops a redundant double BufferedReader).
• java/dereferenced-value-may-be-null — InspectorPanel.Fetcher.run: null-check the logical record before dereferencing it.

Notes

• java/call-to-object-tostring — JoinPolicy: added a toString() returning the policy name. This also repairs a latent bug — JoinPolicy.forName compared against the default Object.toString() and therefore never matched by name, so the joinpolicy configuration property always threw. JoinPolicyTest now covers forName resolution/rejection, and the copy-pasted "No such SplitPolicy" message is corrected.
• java/useless-tostring-call — GetVersion, AdminUITaskPanel: removed toString() calls on values that are already String.
• java/local-variable-is-never-read — BufferPool (read the OOME reserve block before releasing it, keeping the reservation effective), ManagementTableModel (remove the unread minWidth, keeping the load-bearing optional-token parse), ManagementSlidingTableModel (remove the unread firstUpdatedRow).
• java/constants-only-interface — ThreadSequencer: stop implementing SequencerConstants; the constants are unused there and the tests reference them via SequencerConstants.*. ThreadSequencer is public, so external code relying on the inherited constants would need to import SequencerConstants directly — worth a release-notes line.
• java/unused-reference-type — removed the unused VTComboBoxModel class (no references anywhere in the repo).

Both persistit/core and persistit/ui compile cleanly and JoinPolicyTest passes. The structural fixes (array-index, resource-leak) can only be confirmed closed by the next CodeQL scan on master.

[maximthomas]

Re-reviewed after ec09b3d. The Java baseline is fine — the parent pom targets 11 and animal-sniffer was dropped in 0f5e38085, so a Java 9+ API is safe here. But the new commit rests on an unreachability claim that the constants contradict, and that's now the blocker. The other ten files are unchanged from the first round; my earlier points on them stand.

Exchange: the removed guard is reachable, and it was absorbing an off-by-one (moderate)

persistit/core/src/main/java/com/persistit/Exchange.java

final int lvl = level >= 0 ? level : _tree.getDepth() + level;
final LevelCache[] levelCache = _levelCache;
Objects.checkIndex(lvl, levelCache.length);

The commit message says "the check stays redundant given the earlier tree-depth guard … only the exception type on the unreachable branch changes." It's redundant only if _tree.getDepth() <= MAX_TREE_DEPTH (20), and the constants don't guarantee that:

• Tree.setRootPageAddress derives depth from the root page type, validating type <= PAGE_TYPE_INDEX_MAX, then sets version._depth = type - PAGE_TYPE_DATA + 1. With PAGE_TYPE_DATA = 1 and PAGE_TYPE_INDEX_MAX = 21, that yields depth up to 21 — one more than MAX_TREE_DEPTH = 20, which is _levelCache.length.
• Tree.load (Tree.java:316) applies no validation at all: version._depth = Util.getShort(bytes, index + 16);, and Util.getShort sign-extends, so a damaged directory record yields any value in [-32768, 32767].
• Nothing bounds growth either — Tree.changeRootPageAddr just does version._depth += deltaDepth;, and MAX_TREE_DEPTH appears in no guard anywhere (only array sizings and loop bounds). IntegrityCheck.java:889 treats an over-deep tree as a reportable fault (addFault("Tree is too deep", …)), i.e. as possible.

With depth = 21, level = 20 clears the first guard (20 >= 21 is false), lvl = 20, and the second check fires. That's not a hypothetical corner: CLI.java:1044 drives the method straight off the disk value —

final int depth = _currentTree.getDepth();
for (int level = depth; --level >= 0;) {
    final Buffer copy = exchange.fetchBufferCopy(level);

So the caller used to get IllegalArgumentException("Tree depth is 21") and now gets IndexOutOfBoundsException("Index 20 out of bounds for length 20"). Two consequences: the depth disappears from the message on exactly the path used to diagnose a suspect volume, and because IndexOutOfBoundsException is a sibling of IllegalArgumentException rather than a subclass, any catch (IllegalArgumentException) around this public method stops catching it. ManagementImpl.getBufferInfo (ManagementImpl.java:669) catches only PersistitException, so it propagates over RMI to the AdminUI either way.

Simplest resolution: keep the previous commit's version — the local capture plus the original if. I checked it and it is correct and behaviour-preserving (_levelCache is private final … = new LevelCache[MAX_TREE_DEPTH], Exchange.java:267), so if alert #2090 survives it, it is a false positive and dismissing it is defensible. If you'd rather keep the sanitizer, preserve the contract:

final LevelCache[] levelCache = _levelCache;
try {
    Objects.checkIndex(lvl, levelCache.length);
} catch (final IndexOutOfBoundsException e) {
    throw new IllegalArgumentException("Tree depth is " + _tree.getDepth(), e);
}

Either way, please drop the "unreachable" wording from the commit message.

Missing test for the only behavioural change (medium)

JoinPolicy.forName compared policy.toString() against the name, so it matched nothing and always threw. Configuration.setJoinPolicy(String) — the joinpolicy property — was therefore broken for every value:

persistit/core/src/main/java/com/persistit/Configuration.java

public void setJoinPolicy(final String policyName) {
    if (policyName != null) {
        setJoinPolicy(JoinPolicy.forName(policyName));  // always threw before this PR
    }
}

Adding toString() repairs it. Nothing in the repo exercises forName or the joinpolicy property, which is why this went unnoticed. Please lock it in — SplitPolicyTest is the precedent for the location:

persistit/core/src/test/java/com/persistit/JoinPolicyTest.java

@Test
public void forNameResolvesPolicies() {
    assertSame(JoinPolicy.EVEN_BIAS, JoinPolicy.forName("even"));
    assertSame(JoinPolicy.LEFT_BIAS, JoinPolicy.forName("LEFT"));
    assertSame(JoinPolicy.RIGHT_BIAS, JoinPolicy.forName("Right"));
}

@Test(expected = IllegalArgumentException.class)
public void forNameRejectsUnknown() {
    JoinPolicy.forName("NOPE");
}

BufferPool: message churn inside the OOME handler (minor)

persistit/core/src/main/java/com/persistit/BufferPool.java

final int reserved = reserve.length;
reserve = null;
System.err.print("Out of memory after reserving ");
System.err.print(reserved);
System.err.print(" bytes, with ");

reserved is always 1024 * 1024 — a constant, so the new text adds no diagnostic value while adding two more String-allocating print calls to a path whose own comment says it is "written this way to try to avoid another OOME." Reading reserve is what clears the alert; the message change isn't needed.

ThreadSequencer: binary-incompatible for external consumers (minor)

persistit/core/src/main/java/com/persistit/util/ThreadSequencer.java

Safe in-repo — none of the 30 SequencerConstants members are referenced inside ThreadSequencer, there are no wildcard static imports of it, and every constant user imports SequencerConstants directly. But ThreadSequencer is public in a published artifact, so code compiled against ThreadSequencer.COMMIT_FLUSH_A will now fail with NoSuchFieldError. Low risk for an internal test hook; worth a line in the release notes.

Nits

• Exchange — document the thrown exception: fetchBufferCopy has no @throws in its javadoc at all. Whichever exception you settle on, state it — that turns the choice above into a decision rather than an accident.
• Exchange — discarded return value: Objects.checkIndex returns the index; ignoring it may itself attract a java/ignored-return-value-style alert. Worth a glance at the next scan so you don't trade one finding for another.
• JoinPolicy — wrong type in the error message: throw new IllegalArgumentException("No such SplitPolicy " + name); — copy-paste from SplitPolicy, and you're already in the file. persistit/core/src/main/java/com/persistit/policy/JoinPolicy.java
• JoinPolicy — forName could use getName(): comparing policy.getName() clears the same java/call-to-object-tostring alert and can't be broken by a subclass overriding toString() (the ctor is protected). Matching SplitPolicy's existing style is defensible too — your call.
• InspectorPanel — the added guard is unreachable: Fetcher is only constructed at InspectorPanel.java:137, inside refresh(), which already returns early on lr == null; the field is nulled only later in run(). Harmless, but a short "defensive, for static analysis" comment stops someone deleting it again. persistit/ui/src/main/java/com/persistit/ui/InspectorPanel.java
• ManagementTableModel — say that the parse is load-bearing: keeping the token consumption is required, not just validation. TaskStatus.column.3 = getState:100:A:State:30:TaskStatusStateRenderer is the one spec supplying the optional 5th token; dropping the read would shift TaskStatusStateRenderer into the min-width slot and break renderer resolution. Min width is really encoded in the width token (_widths[i] / 10000, line 248), so the local was genuinely dead — the change is right, the comment just undersells why the parseInt must stay. persistit/ui/src/main/java/com/persistit/ui/ManagementTableModel.java

One note on framing: despite the security label, nothing security-relevant changed. The "high" java/improper-validation-of-array-index finding was a false positive — the original bounds check was already correct — so this is a static-analysis legibility fix, not a patched vulnerability. Worth being precise about that in the changelog.

[vharseko]

Thanks for the thorough re-review — you're right on every count. Pushed as a single squashed commit (f18f79e0), with the history reworded to drop the "unreachable" claim.

Exchange — reachable guard / off-by-one

Confirmed. Tree.setRootPageAddress yields depth up to PAGE_TYPE_INDEX_MAX = 21, and Tree.load / changeRootPageAddr are unbounded, so Objects.checkIndex(20, 20) is reachable — CLI drives it straight off the disk value on a suspect volume. Kept the sanitizer but preserved the contract by rethrowing from the IndexOutOfBoundsException:

final LevelCache[] levelCache = _levelCache;
try {
    Objects.checkIndex(lvl, levelCache.length);
} catch (final IndexOutOfBoundsException e) {
    throw new IllegalArgumentException("Tree depth is " + _tree.getDepth(), e);
}

So the depth stays in the message and any catch (IllegalArgumentException) around the public method keeps catching it. Added the missing @throws to the javadoc, and the commit message no longer claims the branch is unreachable.

JoinPolicy — missing test

Added JoinPolicyTest covering forName resolution (incl. case-insensitivity) and rejection of unknown names — green locally. Also fixed the copy-pasted message: "No such SplitPolicy" → "No such JoinPolicy". Left forName comparing on toString() to match SplitPolicy's style; the new toString() returns _name and the test pins it down.

BufferPool — message churn

Reverted the message text back to the original single line; kept only the read of reserve (if (reserve.length > 0) reserve = null;), which is what clears the alert. No extra allocations added to the OOME path.

ThreadSequencer — binary compatibility

Called out in the PR description as a release-notes item. The change is in-repo-safe — no wildcard static imports, and every constant consumer imports SequencerConstants directly.

Nits

• Exchange @throws — done (see above).
• Discarded Objects.checkIndex return — left as-is (pure guard use); I'll watch the next scan in case it attracts a java/ignored-return-value finding.
• InspectorPanel — added a "defensive, for static analysis" comment so the guard isn't deleted again.
• ManagementTableModel — expanded the comment: the parseInt is load-bearing (it consumes the optional 5th token so the renderer name isn't shifted into its slot); the effective minimum width comes from the width token (_widths[i] / 10000).

Framing

Reworded the PR description to say this is a static-analysis legibility fix, not a security fix, and dropped the security label.

[maximthomas]

Re-reviewed f18f79e0. All prior findings are correctly addressed — except the BufferPool fix, which trades one CodeQL alert for a new one. CodeQL already filed the new alert on the branch (05:47, ~2 min after the "fixed" comment), so the PR's own goal (no open alerts) isn't met yet.

BufferPool: the fix reintroduces a CodeQL alert (medium — blocks the PR goal, no runtime impact)

To clear java/local-variable-is-never-read on reserve = null, the handler now reads reserve.length:

if (reserve.length > 0) {
    reserve = null;
}

reserve is new byte[1024 * 1024] (persistit/core/src/main/java/com/persistit/BufferPool.java:272) and is never reassigned before this catch, so reserve.length > 0 is a constant true. CodeQL flagged it as java/useless-comparison-test ("Test is always true") at BufferPool.java:287. Net: one alert closed, one opened.

Preferred fix — stop working around a false positive. reserve = null is a deliberate GC hint (free 1 MB before the diagnostic prints), which the "never read" query can't model. Dismiss that alert as won't fix / intended and restore:

reserve = null;

If a code-only fix is required, read the length as a value (no tautology; matches the existing System.err.print(int) calls in the block):

final int reservedBytes = reserve.length;
reserve = null;
System.err.print("Out of memory (freed ");
System.err.print(reservedBytes);
System.err.print(" reserved bytes) with ");
System.err.print(Runtime.getRuntime().freeMemory());

Nits

• CLI source() (persistit/core/src/main/java/com/persistit/CLI.java:909): dropping the double-BufferedReader is a good cleanup and the reader now escapes into _sourceStack (closed by closeSourceStack()), which should suppress java/input-resource-leak — but no explicit close was added, so confirm the alert clears on the next scan.
• ManagementTableModel parseInt (persistit/ui/src/main/java/com/persistit/ui/ManagementTableModel.java): the discarded Integer.parseInt(...) return may attract java/ignored-return-value later — same "watch" note you put on the discarded Objects.checkIndex return.

[vharseko]

Pushed 467751d1 — took your preferred resolution on both remaining points.

Exchange

Reverted to the local capture plus the original if bounds check; dropped Objects.checkIndex and the now-unused java.util.Objects import:

final int lvl = level >= 0 ? level : _tree.getDepth() + level;
final LevelCache[] levelCache = _levelCache;
if (lvl < 0 || lvl >= levelCache.length) {
    throw new IllegalArgumentException("Tree depth is " + _tree.getDepth());
}
...
final Buffer buffer = levelCache[lvl]._buffer;

This keeps the IllegalArgumentException("Tree depth is N") contract on the reachable over-deep-tree path (depth up to 21 > MAX_TREE_DEPTH, driven straight off the on-disk value by CLI), so the depth stays in the message and catch (IllegalArgumentException) still catches. Kept the @throws javadoc. Since checkIndex is gone, the discarded-return / java/ignored-return-value concern is moot as well.

BufferPool

Restored the plain reserve = null;. The if (reserve.length > 0) was a constant-true test (java/constant-comparison) — exactly the alert-for-alert trade you flagged.

Remaining findings — both false positives, to be dismissed

• java/improper-validation-of-array-index on fetchBufferCopy: the access is provably safe (_levelCache is private final … = new LevelCache[MAX_TREE_DEPTH] and the guard bounds the index). If it survives the next scan I'll dismiss it as a false positive, as you suggested.
• java/local-variable-is-never-read on reserve: the = null is a deliberate GC hint the query can't model — I'll dismiss it as won't-fix / intended.