[
https://issues.apache.org/jira/browse/LUCENE-9306?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Adrian Nistor updated LUCENE-9306:
----------------------------------
Description:
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}
was:
h1. Github Pull Request
I created a pull request on github for this:
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}
> 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
> 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: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
