Skip to content

Commit

Permalink
Protect config object from concurrent modification issues
Browse files Browse the repository at this point in the history
We've had rare reports of modification exceptions from customers around
the config objects.  Using a lock object to protect the internal
collection from modification on other threads and changing the behavoir
of the getter to pass a copy of the collection instead of direct
collection references.

- Resolves opensearch-project#3404

Signed-off-by: Peter Nied <[email protected]>
  • Loading branch information
peternied committed Jan 12, 2024
1 parent b905999 commit 0c7fe39
Showing 1 changed file with 52 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;

Expand All @@ -54,8 +55,10 @@ public class SecurityDynamicConfiguration<T> implements ToXContent {
private static final TypeReference<HashMap<String, Object>> typeRefMSO = new TypeReference<HashMap<String, Object>>() {
};

@JsonIgnore
@JsonIgnoresrc/main/java/org/opensearch/security/securityconf/impl/SecurityDynamicConfiguration.java
private final Map<String, T> centries = new HashMap<>();
@JsonIgnore
private final Object modificationLock = new Object();
private long seqNo = -1;
private long primaryTerm = -1;
private CType ctype;
Expand Down Expand Up @@ -158,23 +161,33 @@ void setCEntries(String key, T value) {

@JsonAnyGetter
public Map<String, T> getCEntries() {
return centries;
synchronized (modificationLock) {
return new HashMap<>(centries);
}
}

@JsonIgnore
public void removeHidden() {
for (Entry<String, T> entry : new HashMap<String, T>(centries).entrySet()) {
if (entry.getValue() instanceof Hideable && ((Hideable) entry.getValue()).isHidden()) {
centries.remove(entry.getKey());
synchronized (modificationLock) {
final Iterator<Entry<String, T>> iterator = centries.entrySet().iterator();
while (iterator.hasNext()) {
final var entry = iterator.next();
if (entry.getValue() instanceof Hideable && ((Hideable) entry.getValue()).isHidden()) {
iterator.remove();
}
}
}
}

@JsonIgnore
public void removeStatic() {
for (Entry<String, T> entry : new HashMap<String, T>(centries).entrySet()) {
if (entry.getValue() instanceof StaticDefinable && ((StaticDefinable) entry.getValue()).isStatic()) {
centries.remove(entry.getKey());
synchronized (modificationLock) {
final Iterator<Entry<String, T>> iterator = centries.entrySet().iterator();
while (iterator.hasNext()) {
final var entry = iterator.next();
if (entry.getValue() instanceof StaticDefinable && ((StaticDefinable) entry.getValue()).isStatic()) {
iterator.remove();
}
}
}
}
Expand All @@ -189,20 +202,26 @@ public void clearHashes() {
}

public void removeOthers(String key) {
T tmp = this.centries.get(key);
this.centries.clear();
this.centries.put(key, tmp);
synchronized (modificationLock) {
T tmp = this.centries.get(key);
this.centries.clear();
this.centries.put(key, tmp);
}
}

@JsonIgnore
public T putCEntry(String key, T value) {
return centries.put(key, value);
synchronized (modificationLock) {
return centries.put(key, value);
}
}

@JsonIgnore
@SuppressWarnings("unchecked")
public void putCObject(String key, Object value) {
centries.put(key, (T) value);
synchronized (modificationLock) {
centries.put(key, (T) value);
}
}

@JsonIgnore
Expand Down Expand Up @@ -286,36 +305,42 @@ public SecurityDynamicConfiguration<T> deepClone() {

@JsonIgnore
public void remove(String key) {
centries.remove(key);
synchronized (modificationLock) {
centries.remove(key);
}
}

@JsonIgnore
public void remove(List<String> keySet) {
keySet.stream().forEach(this::remove);
synchronized (modificationLock) {
keySet.stream().forEach(centries::remove);
}
}

@SuppressWarnings({ "rawtypes", "unchecked" })
public boolean add(SecurityDynamicConfiguration other) {
if (other.ctype == null || !other.ctype.equals(this.ctype)) {
return false;
}
synchronized (modificationLock) {
if (other.ctype == null || !other.ctype.equals(this.ctype)) {
return false;
}

if (other.getImplementingClass() == null || !other.getImplementingClass().equals(this.getImplementingClass())) {
return false;
}
if (other.getImplementingClass() == null || !other.getImplementingClass().equals(this.getImplementingClass())) {
return false;
}

if (other.version != this.version) {
return false;
}
if (other.version != this.version) {
return false;
}

this.centries.putAll(other.centries);
return true;
this.centries.putAll(other.centries);
return true;
}
}

@JsonIgnore
@SuppressWarnings({ "rawtypes" })
public boolean containsAny(SecurityDynamicConfiguration other) {
return !Collections.disjoint(this.centries.keySet(), other.centries.keySet());
return !Collections.disjoint(this.getCEntries().keySet(), other.getCEntries().keySet());
}

public boolean isHidden(String resourceName) {
Expand Down

0 comments on commit 0c7fe39

Please sign in to comment.