commit bde697f44702c3f93cfff7953ff8f5962582e2e1 Author: Karsten Loesing <karsten.loesing@gmx.net> Date: Thu Oct 24 21:29:07 2019 +0200 Make NetworkStatusEntryImpl#parseSLine thread-safe. The bug was that we accessed static class members, namely the two maps NetworkStatusEntryImpl#flagIndexes and #flagStrings, during instance creation without synchronization. This worked just fine with a single thread creating instances, but it breaks with multiple threads doing that at the same time. The fix is to keep a separate map per NetworkStatusImpl instance and share that between all its NetworkStatusEntryImpl instances. This doesn't save as much memory as sharing maps between all NetworksStatusEntryImpl instances ever created, but it's a reasonable compromise between memory and runtime efficiency. In contrast to that, synchronizing map access would have put a major runtime performance penalty on parsing. Fixes #32194. --- CHANGELOG.md | 3 +++ .../descriptor/impl/NetworkStatusEntryImpl.java | 27 +++++++++++----------- .../descriptor/impl/NetworkStatusImpl.java | 8 ++++++- .../impl/RelayNetworkStatusConsensusImpl.java | 3 ++- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c7610a..73022cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Changes in version 2.?.? - 2019-??-?? + * Medium changes + - Make NetworkStatusEntryImpl#parseSLine thread-safe. + # Changes in version 2.8.0 - 2019-10-18 diff --git a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java index 2ddaaa0..e6a78f6 100644 --- a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java @@ -12,7 +12,6 @@ import org.torproject.descriptor.NetworkStatusEntry; import java.util.ArrayList; import java.util.BitSet; import java.util.EnumSet; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -48,13 +47,19 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry { return lines; } + private Map<String, Integer> flagIndexes; + + private Map<Integer, String> flagStrings; + protected NetworkStatusEntryImpl(DescriptorImpl parent, int offset, - int length, boolean microdescConsensus) - throws DescriptorParseException { + int length, boolean microdescConsensus, Map<String, Integer> flagIndexes, + Map<Integer, String> flagStrings) throws DescriptorParseException { this.parent = parent; this.offset = offset; this.length = length; this.microdescConsensus = microdescConsensus; + this.flagIndexes = flagIndexes; + this.flagStrings = flagStrings; this.parseStatusEntryBytes(); this.clearAtMostOnceKeys(); } @@ -160,21 +165,17 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry { this.orAddresses.add(parts[1]); } - private static Map<String, Integer> flagIndexes = new HashMap<>(); - - private static Map<Integer, String> flagStrings = new HashMap<>(); - private void parseSLine(String[] parts) throws DescriptorParseException { this.parsedAtMostOnceKey(Key.S); - BitSet flags = new BitSet(flagIndexes.size()); + BitSet flags = new BitSet(this.flagIndexes.size()); for (int i = 1; i < parts.length; i++) { String flag = parts[i]; - if (!flagIndexes.containsKey(flag)) { - flagStrings.put(flagIndexes.size(), flag); - flagIndexes.put(flag, flagIndexes.size()); + if (!this.flagIndexes.containsKey(flag)) { + this.flagStrings.put(this.flagIndexes.size(), flag); + this.flagIndexes.put(flag, this.flagIndexes.size()); } - flags.set(flagIndexes.get(flag)); + flags.set(this.flagIndexes.get(flag)); } this.flags = flags; } @@ -353,7 +354,7 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry { if (this.flags != null) { for (int i = this.flags.nextSetBit(0); i >= 0; i = this.flags.nextSetBit(i + 1)) { - result.add(flagStrings.get(i)); + result.add(this.flagStrings.get(i)); } } return result; diff --git a/src/main/java/org/torproject/descriptor/impl/NetworkStatusImpl.java b/src/main/java/org/torproject/descriptor/impl/NetworkStatusImpl.java index 0694aff..b763f30 100644 --- a/src/main/java/org/torproject/descriptor/impl/NetworkStatusImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/NetworkStatusImpl.java @@ -10,7 +10,9 @@ import org.torproject.descriptor.NetworkStatusEntry; import java.io.File; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; @@ -19,6 +21,10 @@ import java.util.TreeMap; * delegate the specific parts to the subclasses. */ public abstract class NetworkStatusImpl extends DescriptorImpl { + protected Map<String, Integer> flagIndexes = new HashMap<>(); + + protected Map<Integer, String> flagStrings = new HashMap<>(); + protected NetworkStatusImpl(byte[] rawDescriptorBytes, int[] offsetAndLength, File descriptorFile, boolean containsDirSourceEntries, boolean blankLinesAllowed) throws DescriptorParseException { @@ -140,7 +146,7 @@ public abstract class NetworkStatusImpl extends DescriptorImpl { protected void parseStatusEntry(int offset, int length) throws DescriptorParseException { NetworkStatusEntryImpl statusEntry = new NetworkStatusEntryImpl( - this, offset, length, false); + this, offset, length, false, this.flagIndexes, this.flagStrings); this.statusEntries.put(statusEntry.getFingerprint(), statusEntry); List<String> unrecognizedStatusEntryLines = statusEntry .getAndClearUnrecognizedLines(); diff --git a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java b/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java index 904dc35..66b8eaa 100644 --- a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java @@ -119,7 +119,8 @@ public class RelayNetworkStatusConsensusImpl extends NetworkStatusImpl protected void parseStatusEntry(int offset, int length) throws DescriptorParseException { NetworkStatusEntryImpl statusEntry = new NetworkStatusEntryImpl(this, - offset, length, this.microdescConsensus); + offset, length, this.microdescConsensus, this.flagIndexes, + this.flagStrings); this.statusEntries.put(statusEntry.getFingerprint(), statusEntry); List<String> unrecognizedStatusEntryLines = statusEntry .getAndClearUnrecognizedLines();