From 0b4a9499f08290276e357c8162271bec846a597d Mon Sep 17 00:00:00 2001 From: Alexander Korepanov Date: Mon, 26 Jun 2023 15:08:01 +0200 Subject: [PATCH] [JS IR] Catch concurrent modifications of HashMap ^KT-59001 --- .../src/kotlin/collections/InternalHashMap.kt | 42 ++++++++++++++++++- .../collections/InternalStringLinkedMap.kt | 11 +++++ .../kotlin/collections/InternalStringMap.kt | 30 +++++++++++++ .../collections/ConcurrentModificationTest.kt | 40 +++++++++++++++--- 4 files changed, 116 insertions(+), 7 deletions(-) diff --git a/libraries/stdlib/js/src/kotlin/collections/InternalHashMap.kt b/libraries/stdlib/js/src/kotlin/collections/InternalHashMap.kt index ab6a3cb7a4b..8d99b6aea36 100644 --- a/libraries/stdlib/js/src/kotlin/collections/InternalHashMap.kt +++ b/libraries/stdlib/js/src/kotlin/collections/InternalHashMap.kt @@ -6,15 +6,33 @@ package kotlin.collections internal class InternalHashMap private constructor( + // keys in insert order private var keysArray: Array, - private var valuesArray: Array?, // allocated only when actually used, always null in pure HashSet + // values in insert order, allocated only when actually used, always null in pure HashSet + private var valuesArray: Array?, + // hash of a key by its index, -1 if a key at that index was removed private var presenceArray: IntArray, + // (index + 1) of a key by its hash, 0 if there is no key with that hash, -1 if collision chain continues to the hash-1 private var hashArray: IntArray, + // max length of a collision chain private var maxProbeDistance: Int, - private var length: Int, + // index of the next key to be inserted + private var length: Int ) : InternalMap { private var hashShift: Int = computeShift(hashSize) + /** + * The number of times this map is structurally modified. + * + * A modification is considered to be structural if it changes the map size, + * or otherwise changes it in a way that iterations in progress may return incorrect results. + * + * This value can be used by iterators of the [keys], [values] and [entries] views + * to provide fail-fast behavoir when a concurrent modification is detected during iteration. + * [ConcurrentModificationException] will be thrown in this case. + */ + private var modCount: Int = 0 + private var _size: Int = 0 override val size: Int get() = _size @@ -133,6 +151,7 @@ internal class InternalHashMap private constructor( valuesArray?.resetRange(0, length) _size = 0 length = 0 + registerModification() } override fun equals(other: Any?): Boolean { @@ -169,6 +188,10 @@ internal class InternalHashMap private constructor( private val capacity: Int get() = keysArray.size private val hashSize: Int get() = hashArray.size + private fun registerModification() { + modCount += 1 + } + override fun checkIsMutable() { if (isReadOnly) throw UnsupportedOperationException() } @@ -231,6 +254,7 @@ internal class InternalHashMap private constructor( } private fun rehash(newHashSize: Int) { + registerModification() if (length > _size) compact() if (newHashSize != hashSize) { hashArray = IntArray(newHashSize) @@ -303,6 +327,7 @@ internal class InternalHashMap private constructor( presenceArray[putIndex] = hash hashArray[hash] = putIndex + 1 _size++ + registerModification() if (probeDistance > maxProbeDistance) maxProbeDistance = probeDistance return putIndex } @@ -331,6 +356,7 @@ internal class InternalHashMap private constructor( removeHashAt(presenceArray[index]) presenceArray[index] = TOMBSTONE _size-- + registerModification() } private fun removeHashAt(removedHash: Int) { @@ -469,6 +495,7 @@ internal class InternalHashMap private constructor( ) { internal var index = 0 internal var lastIndex: Int = -1 + private var expectedModCount: Int = map.modCount init { initNext() @@ -482,14 +509,23 @@ internal class InternalHashMap private constructor( fun hasNext(): Boolean = index < map.length fun remove() { + checkForComodification() + check(lastIndex != -1) { "Call next() before removing element from the iterator." } map.checkIsMutable() map.removeKeyAt(lastIndex) lastIndex = -1 + expectedModCount = map.modCount + } + + internal fun checkForComodification() { + if (map.modCount != expectedModCount) + throw ConcurrentModificationException() } } internal class KeysItr(map: InternalHashMap) : Itr(map), MutableIterator { override fun next(): K { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = map.keysArray[lastIndex] @@ -501,6 +537,7 @@ internal class InternalHashMap private constructor( internal class ValuesItr(map: InternalHashMap) : Itr(map), MutableIterator { override fun next(): V { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = map.valuesArray!![lastIndex] @@ -511,6 +548,7 @@ internal class InternalHashMap private constructor( internal class EntriesItr(map: InternalHashMap) : Itr(map), MutableIterator> { override fun next(): EntryRef { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = EntryRef(map, lastIndex) diff --git a/libraries/stdlib/js/src/kotlin/collections/InternalStringLinkedMap.kt b/libraries/stdlib/js/src/kotlin/collections/InternalStringLinkedMap.kt index d2282d5d55a..c25007eba86 100644 --- a/libraries/stdlib/js/src/kotlin/collections/InternalStringLinkedMap.kt +++ b/libraries/stdlib/js/src/kotlin/collections/InternalStringLinkedMap.kt @@ -96,8 +96,10 @@ internal class InternalStringLinkedMap : InternalStringMap() { private abstract class BaseLinkedItr(protected val map: InternalStringLinkedMap) { protected var lastIndex = EMPTY_INDEX protected var index = map.headIndex + private var expectedModCount = map.modCount protected fun goNext() { + checkForComodification() if (index == EMPTY_INDEX) { throw NoSuchElementException() } @@ -108,11 +110,20 @@ internal class InternalStringLinkedMap : InternalStringMap() { fun hasNext(): Boolean = index != EMPTY_INDEX fun remove() { + checkForComodification() + check(lastIndex != EMPTY_INDEX) { "Call next() before removing element from the iterator." } map.removeKeyIndex(map.keys.getElement(lastIndex), lastIndex) if (index == map.size) { index = lastIndex } lastIndex = EMPTY_INDEX + expectedModCount = map.modCount + } + + private fun checkForComodification() { + if (map.modCount != expectedModCount) { + throw ConcurrentModificationException() + } } } diff --git a/libraries/stdlib/js/src/kotlin/collections/InternalStringMap.kt b/libraries/stdlib/js/src/kotlin/collections/InternalStringMap.kt index 7cc56a7f14b..a85ca81a42b 100644 --- a/libraries/stdlib/js/src/kotlin/collections/InternalStringMap.kt +++ b/libraries/stdlib/js/src/kotlin/collections/InternalStringMap.kt @@ -54,6 +54,22 @@ internal open class InternalStringMap : InternalMap { internal var values = createJsArray() internal var keys = createJsArray() + /** + * The number of times this map is structurally modified. + * + * A modification is considered to be structural if it changes the map size, + * or otherwise changes it in a way that iterations in progress may return incorrect results. + * + * This value can be used by iterators of the [keys], [values] and [entries] views + * to provide fail-fast behavoir when a concurrent modification is detected during iteration. + * [ConcurrentModificationException] will be thrown in this case. + */ + internal var modCount: Int = 0 + + private fun registerModification() { + modCount += 1 + } + override val size: Int get() = keys.length @@ -121,6 +137,7 @@ internal open class InternalStringMap : InternalMap { backingMap[key] = size keys.push(key) values.push(value) + registerModification() return null } @@ -155,12 +172,14 @@ internal open class InternalStringMap : InternalMap { values.replaceElementAtWithLast(removingIndex) backingMap[keys.getElement(removingIndex)] = removingIndex } + registerModification() } override fun clear() { backingMap = createJsMap() keys = createJsArray() values = createJsArray() + registerModification() } override fun build() { @@ -177,8 +196,10 @@ internal open class InternalStringMap : InternalMap { private abstract class BaseItr(protected val map: InternalStringMap) { protected var lastIndex = -1 protected var index = 0 + private var expectedModCount = map.modCount protected fun goNext() { + checkForComodification() if (index >= map.size) { throw NoSuchElementException() } @@ -188,9 +209,18 @@ internal open class InternalStringMap : InternalMap { fun hasNext(): Boolean = index < map.size fun remove() { + checkForComodification() + check(lastIndex != -1) { "Call next() before removing element from the iterator." } map.removeKeyIndex(map.keys.getElement(lastIndex), lastIndex) index = lastIndex lastIndex = -1 + expectedModCount = map.modCount + } + + private fun checkForComodification() { + if (map.modCount != expectedModCount) { + throw ConcurrentModificationException() + } } } diff --git a/libraries/stdlib/test/collections/ConcurrentModificationTest.kt b/libraries/stdlib/test/collections/ConcurrentModificationTest.kt index 252cf843777..58d9619647d 100644 --- a/libraries/stdlib/test/collections/ConcurrentModificationTest.kt +++ b/libraries/stdlib/test/collections/ConcurrentModificationTest.kt @@ -6,6 +6,10 @@ package test.collections import test.TestPlatform +import test.collections.js.linkedStringMapOf +import test.collections.js.linkedStringSetOf +import test.collections.js.stringMapOf +import test.collections.js.stringSetOf import test.current import kotlin.test.Test import kotlin.test.assertFailsWith @@ -167,8 +171,6 @@ class ConcurrentModificationTest { @Test fun mutableSet() { - if (TestPlatform.current == TestPlatform.Js) return - val operations = listOf>>( CollectionOperation("add(non-existing)") { add("e") }, CollectionOperation("add(existing)", throwsCME = false) { add("d") }, @@ -226,12 +228,25 @@ class ConcurrentModificationTest { action(this) } } + + if (TestPlatform.current == TestPlatform.Js) { + testThrowsCME { action -> + stringSetOf().apply { + addAll(elements) + action(this) + } + } + testThrowsCME { action -> + linkedStringSetOf().apply { + addAll(elements) + action(this) + } + } + } } @Test fun mutableMap() { - if (TestPlatform.current == TestPlatform.Js) return - val operations = listOf>>( CollectionOperation("put(non-existing)") { put("e", "e") }, CollectionOperation("put(existing)", throwsCME = false) { put("d", "d") }, @@ -285,6 +300,21 @@ class ConcurrentModificationTest { action(this) } } + + if (TestPlatform.current == TestPlatform.Js) { + testThrowsCME { action -> + stringMapOf().apply { + putAll(entries) + action(this) + } + } + testThrowsCME { action -> + linkedStringMapOf().apply { + putAll(entries) + action(this) + } + } + } } } @@ -311,4 +341,4 @@ private val listIteratorOperations = listOf