Commit bde697f4 authored by Karsten Loesing's avatar Karsten Loesing
Browse files

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.
parent 02e486d1
# Changes in version 2.?.? - 2019-??-??
* Medium changes
- Make NetworkStatusEntryImpl#parseSLine thread-safe.
# Changes in version 2.8.0 - 2019-10-18
......
......@@ -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;
......
......@@ -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();
......
......@@ -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();
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment