adriannistor opened a new pull request #1413: The check against null and usage of the field updateThread is typically protected by synchronization, but not in 1 location. URL: https://github.com/apache/lucene-solr/pull/1413 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](https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L339-L341), [357-358](https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L357-L358), and [380-381](https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L380-L381). However, the `null` check at [line 387](https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L387) and the usage at [line 388](https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L388) are not protected by `synchronized`. ```java 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](https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L369)) 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](https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L369), 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](https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L387-L388) `synchronized`, just like the other methods containing `null` checks and usages of `updateThread`. ```java public synchronized String toString() { <<<<<<<<<<<<<< added "synchronized" here String res = "ReplicationClient"; if (updateThread != null) { res += " (" + updateThread.getName() + ")"; ```
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
