[
https://issues.apache.org/jira/browse/LUCENE-9306?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17076726#comment-17076726
]
Adrian Nistor commented on LUCENE-9306:
---------------------------------------
> The patch doesn't appear to include any new or modified tests. Please justify
> why no new tests are needed for this patch. Also please list what manual
> steps were performed to verify this patch.
This bug was found through code inspection.
However, this is a 1-keyword simple patch and all the existing tests pass.
> The check against null and usage of the field updateThread is typically
> protected by synchronization, but not in 1 location.
> ----------------------------------------------------------------------------------------------------------------------------
>
> Key: LUCENE-9306
> URL: https://issues.apache.org/jira/browse/LUCENE-9306
> Project: Lucene - Core
> Issue Type: Bug
> Components: modules/replicator
> Reporter: Adrian Nistor
> Priority: Major
> Attachments: LUCENE-9306.patch
>
>
> h1. Github Pull Request
> I created a pull request on github for this issue at:
> [https://github.com/apache/lucene-solr/pull/1413]
> h1. 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|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}}.
> {code:java}
> public String toString() {
> String res = "ReplicationClient";
> if (updateThread != null) { <<<<<<<<<<<<<<<<<<<<<
> res += " (" + updateThread.getName() + ")"; <<<<<<<<<<<<<<<<<
> {code}
> 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.
> h1. 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}}.
> {code:java}
> public synchronized String toString() { <<<<<<<<<<<<<< added "synchronized"
> here
> String res = "ReplicationClient";
> if (updateThread != null) {
> res += " (" + updateThread.getName() + ")";
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
