[LUCENE-9306] The check against null and usage of the field updateThread is typically protected by synchronization, but not in 1 location.

• Type: Bug
• Status: Patch Available
• Priority: Major
• Resolution: Unresolved
• Component/s: modules/replicator
• Lucene Fields: New, Patch Available
• Assignee: Unassigned
• Reporter: Adrian Nistor
• Created: 06/Apr/20 19:23
• Updated: 28/Aug/22 16:01

Description

Github Pull Request

I created a pull request on github for this issue at:

https://github.com/apache/lucene-solr/pull/1413

Description

Checks against null and usages of the field updateThread are typically made atomic by synchronization on this (synchronized methods), e.g., at lines: 339-341, 357-358, and 380-381.

However, the null check at line 387 and the usage at line 388 are not protected by synchronized.

public String toString() {
  String res = "ReplicationClient";
  if (updateThread != null) { <<<<<<<<<<<<<<<<<<<<<
    res += " (" + updateThread.getName() + ")"; <<<<<<<<<<<<<<<<<

This check against null and usage of updateThread are in toString().

However, the problem is not that the toString() will give a garbled string (i.e., a relatively minor issues).

The problem is that, in between the null check and the usage, updateThread can be set to null (e.g., by line 369) and therefore the code can crash.

I.e., without synchronized, the null check does not protect the updateThread.getName() usage.

I don't know how toString() is called concurrently. However, it seems like a dangerous assumption to make that the callers of toString() know it should not be called concurrently with line 369, especially as the other methods do protect with synchronization the null check and usage.

This Patch's Code

The fix is very simple: just make the method containing lines 387-388 synchronized, just like the other methods containing null checks and usages of updateThread.

public synchronized String toString() { <<<<<<<<<<<<<< added "synchronized" here
  String res = "ReplicationClient";
  if (updateThread != null) {
    res += " (" + updateThread.getName() + ")";