Skip to content

fix(ReindexThread): TOCTOU race on BulkProcessor rebuild — use AtomicReference + compareAndSet #35313

@fabrizzio-dotCMS

Description

@fabrizzio-dotCMS

Description

ReindexThread.java lines 243–248 contains a pre-existing TOCTOU (Time-Of-Check To Time-Of-Use) race condition on the bulk-processor rebuild path.

if (bulkProcessor == null || rebuildBulkIndexer.get()) {   // CHECK
    closeBulkProcessor(bulkProcessor);
    bulkProcessorListener = new BulkProcessorListener();
    bulkProcessor = indexAPI.createBulkProcessor(bulkProcessorListener); // USE
}
bulkProcessorListener.workingRecords.putAll(workingRecords); // line 248

Two threads racing through the null-check can both create a new processor. The first processor is discarded, but any in-flight afterBulk callbacks still hold a reference to the old bulkProcessorListener whose workingRecords has already been replaced — potentially losing reindex records silently.

Additionally, bulkProcessorListener.workingRecords.putAll(workingRecords) at line 248 runs on the ReindexThread main loop while afterBulk callbacks can call workingRecords.get() on the processor's internal thread simultaneously.

Identified during review of PR #35289 by @mbiuki. Left out of scope there because it affects pre-existing code not changed by that PR.

Acceptance Criteria

  • bulkProcessor is held in an AtomicReference<BulkProcessor> and replaced via compareAndSet to make the check+create atomic
  • bulkProcessorListener assignment is coordinated with the processor swap so no callback can observe a mismatched listener
  • Existing reindex integration tests pass (no ConcurrentModificationException, no silent record loss)
  • A unit or integration test covers the concurrent-rebuild scenario (or a comment documents why the race is now impossible)

Additional Context

  • Identified in: dotCMS/src/main/java/com/dotmarketing/common/reindex/ReindexThread.java lines 243–248
  • Related PR: task(Migrate ESMappingAPIImpl) Refs #34933 #35289
  • The companion fix (workingRecordsConcurrentHashMap, contentletsIndexedvolatile) was already applied in commit 3fd612924c

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

Status

New

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions