Skip to content

Fix CodeQL error-level findings: dead branches, log arguments, locking, clone - #205

Open
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-error-alerts
Open

vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-error-alerts

Conversation

@vharseko

Copy link
Copy Markdown
Member

Summary

Closes all 25 open error-level (code quality) CodeQL alerts: #470–#486, #490–#494, #497–#499. Most are mechanical, but three turned out to be real bugs (marked bug below).

Alerts File Change
#477–#486 contradictory-type-checks ReconciliationService Removed the five unreachable else if (… instanceof ScheduledThreadPoolExecutor) branches — it is a ThreadPoolExecutor, so the first branch always wins.
#470–#476 missing-format-argument ClusterManager, ConnectorInfoProviderService, Id, GenericTableHandler, MappedTableHandler ×2, PostgreSQLMappedTableHandler Placeholders and arguments reconciled: missing instanceId; new Object[]{…}, e → plain varargs with the throwable last; stray {} dropped; the "Unprepared query strings" debug line was missing readForUpdateQueryStr.
#497 type-mismatch-accessbug ExplicitResultSetMapper columnMappings.contains("total") compared a String against a List<ColumnMapping> and was always false, so an explicitly mapped total column was silently overwritten with rs.getInt("total"). Now checks the mapped DB column names.
#493/#494 unsafe-sync-on-field / unsafe-double-checked-lockingbug AbstractScriptedService synchronized (selfRegistration) while selfRegistration == nullNullPointerException on every REGISTERED script event (the new test reproduced it). Now a dedicated registrationLock, a volatile field, and the field is cleared after unregister() so a script reload re-registers instead of leaving a stale registration.
#491 missing-clone-methodbug JsonUser JsonValue.clone() returns a plain JsonValue, so cloning a JsonUser lost the User type and its CryptoService. Overridden.
#490 missing-clone-method ChecksumFile clone() override (shallow, like HashMap; documented).
#492 sleep-with-lock-held OrientDBRepoService The 100 ms back-off between pool-acquire retries now happens outside dbLock; only the pool.acquire call is serialised. InterruptedException restores the interrupt flag and stops retrying instead of being swallowed.
#498/#499 unused-container OperationHelperImpl, RepoJobStore Removed resultList (only referenced from commented-out code) and blockedJobs (added/removed, never read) plus the else {} that became empty.

Test plan

  • New ExplicitResultSetMapperTest (2), AbstractScriptedServiceTest (3), JsonUserTest (2), ChecksumFileTest.testCloneKeepsEntriesAndType — all failed before the change (2× compile error, 1× NPE, 1× overwritten value), pass now
  • Full test suites of the 10 touched modules: core 23, cluster 3, provisioner-openicf 142, provisioner 5, repo-jdbc 3, maintenance 116, workflow-activiti 3, repo-orientdb 31, script 11, quartz-fragment 16 — 0 failures
  • CodeQL on this PR closes #470–#486, #490–#494, #497–#499

…g, clone

- ReconciliationService: drop the unreachable ScheduledThreadPoolExecutor
  branches (it is a ThreadPoolExecutor)
- Fix SLF4J placeholder/argument mismatches in ClusterManager,
  ConnectorInfoProviderService, Id, GenericTableHandler,
  MappedTableHandler and PostgreSQLMappedTableHandler
- ExplicitResultSetMapper: the "total" guard compared a String against a
  List<ColumnMapping> and never matched; check the mapped DB column names
- AbstractScriptedService: synchronize on a dedicated lock instead of the
  (possibly null) registration field, make it volatile and clear it after
  unregistering; the old code threw NPE on every REGISTERED script event
- JsonUser / ChecksumFile: override clone() so the copy keeps its type
  (and, for JsonUser, its CryptoService)
- OrientDBRepoService: back off between pool-acquire retries outside the
  pool lock
- Remove the write-only containers in OperationHelperImpl and RepoJobStore

Resolves CodeQL alerts #470-#486, #490-#494, #497-#499.
@vharseko vharseko added java Pull requests that update Java code test Tests and test infrastructure (unit, e2e, smoke) bug Something isn't working refactor Code refactoring without behavior change repo Storage / repository layer (OrientDB, JDBC, HSQLDB) concurrency Thread-safety, locking and synchronization issues labels Sep 18, 2026
vharseko added a commit to vharseko/OpenIDM that referenced this pull request Sep 19, 2026
…correctness

Brings in the JsonUser.clone() override from OpenIdentityPlatform#205 so the error-level
CodeQL alert java/missing-clone-method no longer fires on the class
declaration line this branch touches.

# Conflicts:
#	openidm-workflow-activiti/src/main/java/org/forgerock/openidm/workflow/activiti/impl/JsonUser.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working concurrency Thread-safety, locking and synchronization issues java Pull requests that update Java code refactor Code refactoring without behavior change repo Storage / repository layer (OrientDB, JDBC, HSQLDB) test Tests and test infrastructure (unit, e2e, smoke)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant