Skip to content

Fix null-pointer dereferences flagged by CodeQL java/dereferenced-value-may-be-null#225

Open
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/codeql-dereferenced-null
Open

Fix null-pointer dereferences flagged by CodeQL java/dereferenced-value-may-be-null#225
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/codeql-dereferenced-null

Conversation

@vharseko

@vharseko vharseko commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Resolves 16 real java/dereferenced-value-may-be-null CodeQL alerts by fixing
genuine null-pointer dereferences. Each is a place where a value that can be null
— per an explicit guard elsewhere, or an unassigned local — is dereferenced.

Fixes

Module File Defect
rest/api-descriptor AsciiDocTable Range check unboxed this.columnsPerRow (provably null there) instead of the int parameter — the setter always threw NPE.
json-web-token JWK The new JsonException(...) guarding a null kty was never thrown, so kty reached kty.toString() (NPE). Added the missing throw (kty is required per RFC 7517 §4.1). kid is optional (RFC 7517 §4.5), so its dead exception is dropped and it is only stored when present.
audit/core DateUtil Integer result, returned as int, was null for null input dates → unboxing NPE. Now throws IllegalArgumentException.
script/common ScriptRegistryImpl Guard used || instead of &&, so source/dependency could be null when dereferenced.
script/common ScriptExecutor Empty if (null == scriptEngine) body fell through to .eval(). Now throws ScriptException.
persistit/core ManagementImpl finally dereferenced a possibly-unassigned exchange, masking the real exception. Guarded with a null check.
persistit/core DefaultObjectCoder Two catch handlers called accessor.toString() with accessor possibly null. Use String.valueOf(accessor).
persistit/ui AdminUI newPanel.setDefaultButton() outside its null guard; _buttonList.size() after a null-tolerating block.
persistit/ui AdminUITreePanel treeInfoArray.length without a null check.
persistit/core Exchange Traversal loop dereferenced buffer without the null guard applied just above; added && buffer != null to the loop condition.
persistit/core RecoveryManager lastRequiredPageNode (set in lockstep with the gating address) dereferenced; guarded the validate() call to make the invariant explicit.
persistit/core BufferPool invalidateSmallVolume (per-page) requires a concrete volume; the copy-pasted || volume == null match clauses were dead code that made the analyzer treat volume as nullable. Removed them — behaviour-identical for a real volume; the live "invalidate all" idiom stays in invalidateLargeVolume.
persistit/ui InspectorPanel Fetcher re-read the record field; pass the already null-checked local instead — removes the potential NPE and a TOCTOU race.

Behaviour

Non-error paths are unchanged; the changes only avoid NPEs that were either
guaranteed (AsciiDocTable, JWK) or latent on error/edge paths. JWK keeps
kid optional (RFC 7517 §4.5) so the keyless JWKs built from the JWE spec
examples continue to parse.

Not in this PR

Of the 25 alerts for this rule, the other 9 are high-confidence false positives
(correlated invariants CodeQL cannot prove — e.g. boxed primitives in
ValueInspectorTreeNode, a corrupt() that always throws in Exchange) and were
dismissed on GitHub.

Testing

mvn -o -pl commons/rest/api-descriptor,commons/json-web-token,commons/audit/core,script/common,persistit/core,persistit/ui compile — all six modules compile.
mvn -o -pl commons/json-web-token test — 328 tests, 0 failures, 0 errors.

@vharseko vharseko added codeql CodeQL static-analysis findings bug java labels Jul 8, 2026
…ue-may-be-null

Fix 12 genuine null-dereference defects across commons, script and persistit.
In each case a value that can be null - per an explicit guard elsewhere or an
unassigned local - was dereferenced.

- AsciiDocTable.columnsPerRow: the range check dereferenced the Integer field
  this.columnsPerRow, which is provably null at that point (the preceding guard
  returns otherwise), so the setter always threw NPE. Check the int parameter.
- JWK constructor: the "new JsonException(...)" guarding a null kty was never
  thrown, so kty fell through to kty.toString() and NPE'd. Add the missing throw;
  kty is required (RFC 7517 section 4.1). kid is optional (RFC 7517 section 4.5),
  so drop its dead exception and only store it when present, rather than making it
  mandatory - a mandatory kid rejects the keyless JWKs used by the JWE spec
  examples (RSAEncryptionHandlerTest).
- DateUtil.getDateDifferenceInDays: the Integer result stayed null for null input
  dates and was unboxed on return. Throw IllegalArgumentException instead.
- ScriptRegistryImpl.isDependOn: the guard used || instead of &&, so source or
  dependency could be null when dereferenced. Use &&.
- ScriptExecutor.execute: the "null == scriptEngine" check had an empty body and
  fell through to scriptEngine.eval(). Throw ScriptException.
- ManagementImpl.getLogicalRecordArray: the finally block dereferenced a possibly
  unassigned exchange, masking the original exception. Guard with a null check.
- DefaultObjectCoder key encode/decode: the catch handlers called
  accessor.toString() when accessor could still be null. Use String.valueOf.
- AdminUI: newPanel.setDefaultButton() ran outside the newPanel != null guard, and
  _buttonList.size() ran after a "_buttonList != null" block that could leave it
  null. Move/fold both into their guards.
- AdminUITreePanel: treeInfoArray was dereferenced without a null check. Add one.
- Exchange traversal loop: buffer was dereferenced without the null guard already
  applied just above it; add "&& buffer != null" so the loop ends cleanly.
- RecoveryManager: lastRequiredPageNode is set in lockstep with the address that
  gates this block, so it is non-null here; guard the validate() call to make that
  explicit.
- BufferPool.invalidateSmallVolume: this per-page variant requires a concrete
  volume, so the copy-pasted "|| volume == null" match clauses were dead code that
  made the analyzer treat volume as nullable at the loop bound. Remove them; the
  live "invalidate all" idiom stays in invalidateLargeVolume.
- InspectorPanel.refresh: pass the already null-checked local record to Fetcher
  instead of re-reading the field, removing both the potential NPE and a race.

Behaviour is unchanged on the non-error paths; only guaranteed or latent NPEs are
avoided. The remaining alerts for this rule are dismissed as false positives
(correlated invariants the analyzer cannot prove).
@vharseko vharseko force-pushed the features/codeql-dereferenced-null branch from a911e42 to e2770c0 Compare July 8, 2026 10:15
@vharseko vharseko requested a review from maximthomas July 8, 2026 12:44
@vharseko vharseko removed the java label Jul 8, 2026
@vharseko vharseko force-pushed the features/codeql-dereferenced-null branch from ee2887e to 31e9c6d Compare July 10, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug codeql CodeQL static-analysis findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants