diff --git a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java index c33fffaa..d9d030a6 100644 --- a/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java +++ b/model/src/main/java/org/gorpipe/gor/table/dictionary/DictionaryEntries.java @@ -45,6 +45,10 @@ * serialized and must NOT run concurrently with reads on the same instance (structural * modification of the backing list would break in-flight iteration); share a read-only instance * for concurrent reads and use a separate instance for writes. + * + *

The public read accessors ({@code getEntries()}/{@code getAllActiveTags()}) return + * unmodifiable views so callers cannot structurally modify the shared internal state; + * copy the result if a mutable list is needed (see {@code selectAll()}). */ public class DictionaryEntries implements IDictionaryEntries { private static final Logger log = LoggerFactory.getLogger(DictionaryEntries.class); @@ -94,7 +98,7 @@ synchronized public void insert(T line, boolean hasUniqueTags) { delete(oldLine, true); } - getEntries().add(line); + loadedEntries().add(line); addEntryToContentMap(line); addEntryToTagMap(line); @@ -110,7 +114,7 @@ synchronized public void insert(T line, boolean hasUniqueTags) { synchronized public void delete(T lineToRemove, boolean keepIfBucket) { T line = this.findLine(lineToRemove); if (line != null) { - getEntries().remove(line); + loadedEntries().remove(line); removeEntryFromContentMap(line); removeEntryFromTagMap(line); @@ -118,7 +122,7 @@ synchronized public void delete(T lineToRemove, boolean keepIfBucket) { // NOTE: the deleted flag is part of the hashCode so we remove and add again if we change it. T entry = (T) DictionaryEntry.copy(line); entry.setDeleted(true); - getEntries().add(entry); + loadedEntries().add(entry); addEntryToContentMap(entry); addEntryToTagMap(entry); } @@ -127,6 +131,8 @@ synchronized public void delete(T lineToRemove, boolean keepIfBucket) { @Override synchronized public void clear() { + // We deliberatly set list/maps to null, instead of clearing. This is because someone could be reading + // the buffers and we want to let them finish. dataLoaded = false; this.rawLines = null; clearContentMap(); @@ -135,16 +141,32 @@ synchronized public void clear() { @Override public List getEntries() { - if (!this.dataLoaded) { + // Return an unmodifiable view: the backing list is shared across concurrent readers and + // must not be structurally modified from the outside (see class-level thread-safety note). + return Collections.unmodifiableList(loadedEntries()); + } + + /** + * Load the entries if needed and return the live backing list. + * + *

Reads {@code rawLines} into a single local so the load check and the returned reference are + * the same read; a separate re-read of the volatile field could observe {@code null} if a + * concurrent {@code clear()} slipped in (which the contract forbids, but the local read makes the + * accessor robust anyway). For internal use only — mutators hold the instance lock; read + * accessors that expose the result to callers must wrap it (see {@link #getEntries()}). + */ + private List loadedEntries() { + List lines = this.rawLines; + if (!this.dataLoaded || lines == null) { loadLinesAndUpdateIndices(); + lines = this.rawLines; } - - return this.rawLines; + return lines; } @Override public List getEntries(String... aliasesAndTags) { - List lines2Search = getEntries(); + List lines2Search = loadedEntries(); updateTagMap(); if (tagHashToLines != null && aliasesAndTags != null) { // If we have tags and tag map to lines we use that to get a better list of lines to search.. @@ -195,7 +217,10 @@ public boolean isLoaded() { @Override public Set getAllActiveTags() { updateContentMap(); - return activeTags.elementSet(); + // Read the volatile once and wrap: elementSet() is a live view of the internal multiset and + // supports remove()/clear(), so it must be made unmodifiable before it escapes to callers. + Multiset tags = this.activeTags; + return tags != null ? Collections.unmodifiableSet(tags.elementSet()) : Collections.emptySet(); } @Override @@ -206,13 +231,13 @@ public boolean hasDeletedEntries() { @Override public int size() { - return getEntries().size(); + return loadedEntries().size(); } @Override public int getActiveLinesCount() { updateContentMap(); - int size = getEntries().size(); + int size = loadedEntries().size(); return size - deletedEntriesCount; } @@ -233,6 +258,9 @@ synchronized private void updateContentMap() { synchronized private void updateTagMap() { if (tagHashLoaded) return; + if (!dataLoaded) { + loadLinesAndUpdateIndices(); + } tagHashToLines = ArrayListMultimap.create(rawLines.size(), 1); for (T entry : rawLines) { addEntryToTagMap(entry); @@ -350,7 +378,7 @@ private List loadLines() { @Override public T findLine(T line) { // Using contentHashMap - List allLines = getEntries(); + List allLines = loadedEntries(); updateContentMap(); List lines2Search = contentHashToLines != null ? contentHashToLines.get(line.getSearchHash()) : allLines; @@ -361,7 +389,7 @@ private T findLineWithTag(T line) { String[] linekey = line.getFilterTags(); // Using contentHashMap - List lines2Search = getEntries(); + List lines2Search = loadedEntries(); if (tagHashToLines != null) { lines2Search = new ArrayList<>(); for (String tag : linekey) { diff --git a/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesViews.java b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesViews.java new file mode 100644 index 00000000..b0fd87ad --- /dev/null +++ b/model/src/test/java/org/gorpipe/gor/table/dictionary/UTestDictionaryEntriesViews.java @@ -0,0 +1,96 @@ +package org.gorpipe.gor.table.dictionary; + +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryEntry; +import org.gorpipe.gor.table.dictionary.gor.GorDictionaryTable; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.lang.reflect.Field; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Set; + +/** + * Deterministic regression tests for the read-accessor hardening in {@link DictionaryEntries}: + *

+ */ +public class UTestDictionaryEntriesViews { + + @Rule + public TemporaryFolder workDir = new TemporaryFolder(); + + private GorDictionaryTable buildTable() throws Exception { + Path dir = workDir.getRoot().toPath(); + Files.writeString(dir.resolve("a.gor"), "chrom\tpos\nchr1\t1\n"); + Files.writeString(dir.resolve("b.gor"), "chrom\tpos\nchr1\t2\n"); + Path gord = dir.resolve("t.gord"); + Files.writeString(gord, "a.gor\tpn1\nb.gor\tpn2\n"); + return new GorDictionaryTable.Builder<>(gord.toString()).build(); + } + + @SuppressWarnings("unchecked") + private IDictionaryEntries entriesOf(GorDictionaryTable table) throws Exception { + Field f = DictionaryTableReader.class.getDeclaredField("tableEntries"); + f.setAccessible(true); + return (IDictionaryEntries) f.get(table); + } + + @Test(expected = UnsupportedOperationException.class) + public void getEntries_returnsUnmodifiableList() throws Exception { + IDictionaryEntries entries = entriesOf(buildTable()); + List list = entries.getEntries(); + Assert.assertEquals(2, list.size()); + list.clear(); // must throw - callers must not mutate the shared backing list + } + + @Test(expected = UnsupportedOperationException.class) + public void getAllActiveTags_returnsUnmodifiableSet() throws Exception { + IDictionaryEntries entries = entriesOf(buildTable()); + Set tags = entries.getAllActiveTags(); + Assert.assertTrue(tags.contains("pn1")); + // Guava's Multiset.elementSet() blocks add() but SUPPORTS remove()/clear(), which would + // strip occurrences from the internal multiset. The returned view must reject removal too. + tags.clear(); // must throw - live view of internal multiset must not be mutable + } + + @Test + public void insertAndDelete_stillMutateBackingList() throws Exception { + GorDictionaryTable table = buildTable(); + IDictionaryEntries entries = entriesOf(table); + + Assert.assertEquals(2, entries.size()); + + GorDictionaryEntry newEntry = + (GorDictionaryEntry) new GorDictionaryEntry.Builder("c.gor", table.getRootPath()).alias("pn3").build(); + entries.insert(newEntry, false); + Assert.assertEquals(3, entries.size()); + + entries.delete(newEntry, false); + Assert.assertEquals(2, entries.size()); + } + + @Test + public void getEntriesByTag_reloadsAfterClear() throws Exception { + GorDictionaryTable table = buildTable(); + IDictionaryEntries entries = entriesOf(table); + + Assert.assertEquals(1, entries.getEntries("pn1").size()); + + entries.clear(); + + // After clear the backing list is dropped; the tag-filtered read must reload it, + // not dereference a null rawLines (finding #2). + List afterClear = entries.getEntries("pn1"); + Assert.assertEquals(1, afterClear.size()); + } +}