Fixed concurrency bug that could be triggered via the React Native pl…

…ugin
bugsnag · May 9, 2022 · c25cd04 · c25cd04
1 parent 807a5ca
commit c25cd04
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,12 @@
 # Changelog
 
+## TBD
+
+### Bug fixes
+
+* Fixed concurrency bug that could be triggered via the React Native plugin
+  [#1679](https://github.com/bugsnag/bugsnag-android/pull/1679)
+
 ## 5.22.2 (2022-05-04)
 
 ### Bug fixes

diff --git a/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/DataCollectorTest.kt b/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/DataCollectorTest.kt
@@ -0,0 +1,49 @@
+package com.bugsnag.android
+
+import android.content.Context
+import android.content.res.Configuration
+import android.content.res.Resources
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.mockito.Mockito
+import org.mockito.junit.MockitoJUnitRunner
+import java.io.File
+import kotlin.concurrent.thread
+
+@RunWith(MockitoJUnitRunner::class)
+class DataCollectorTest {
+
+    @Test
+    fun testConcurretAccess() {
+        val res = Mockito.mock(Resources::class.java)
+        Mockito.`when`(res.configuration).thenReturn(Configuration())
+
+        val collector = DeviceDataCollector(
+            Mockito.mock(Connectivity::class.java),
+            Mockito.mock(Context::class.java),
+            res,
+            "fakeDevice",
+            Mockito.mock(DeviceBuildInfo::class.java),
+            File("/tmp/javatest"),
+            Mockito.mock(RootDetector::class.java),
+            Mockito.mock(BackgroundTaskService::class.java),
+            Mockito.mock(Logger::class.java)
+        )
+
+        repeat(10) { index ->
+            collector.addRuntimeVersionInfo("key" + index, "value" + index)
+        }
+
+        val count = 500
+
+        thread {
+            repeat(count) { index ->
+                collector.addRuntimeVersionInfo("key" + index, "value" + index)
+            }
+        }
+
+        repeat(count) {
+            collector.generateDevice()
+        }
+    }
+}
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/BackgroundTaskService.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/BackgroundTaskService.kt
@@ -83,7 +83,7 @@ internal fun createExecutor(name: String, keepAlive: Boolean): ThreadPoolExecuto
  * It also avoids short-running operations being held up by long-running operations submitted
  * to the same executor.
  */
-internal class BackgroundTaskService(
+internal open class BackgroundTaskService(
     // these executors must remain single-threaded - the SDK makes assumptions
     // about synchronization based on this.
     @VisibleForTesting
@@ -137,7 +137,7 @@ internal class BackgroundTaskService(
      * @see [submitTask]
      */
     @Throws(RejectedExecutionException::class)
-    fun <T> submitTask(taskType: TaskType, callable: Callable<T>): Future<T> {
+    open fun <T> submitTask(taskType: TaskType, callable: Callable<T>): Future<T> {
         return when (taskType) {
             TaskType.ERROR_REQUEST -> errorExecutor.submit(callable)
             TaskType.SESSION_REQUEST -> sessionExecutor.submit(callable)

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/DeviceBuildInfo.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/DeviceBuildInfo.kt
@@ -2,7 +2,7 @@ package com.bugsnag.android
 
 import android.os.Build
 
-internal class DeviceBuildInfo(
+internal open class DeviceBuildInfo(
     val manufacturer: String?,
     val model: String?,
     val osVersion: String?,

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/DeviceDataCollector.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/DeviceDataCollector.kt
@@ -42,7 +42,7 @@ internal class DeviceDataCollector(
     private val screenResolution = getScreenResolution()
     private val locale = Locale.getDefault().toString()
     private val cpuAbi = getCpuAbi()
-    private val runtimeVersions: MutableMap<String, Any>
+    private var runtimeVersions: MutableMap<String, Any>
     private val rootedFuture: Future<Boolean>?
     private val totalMemoryFuture: Future<Long?>? = retrieveTotalDeviceMemory()
     private var orientation = AtomicInteger(resources.configuration.orientation)
@@ -293,6 +293,9 @@ internal class DeviceDataCollector(
     }
 
     fun addRuntimeVersionInfo(key: String, value: String) {
-        runtimeVersions[key] = value
+        // Use copy-on-write to avoid a ConcurrentModificationException in generateDeviceWithState
+        val newRuntimeVersions = runtimeVersions.toMutableMap()
+        newRuntimeVersions[key] = value
+        runtimeVersions = newRuntimeVersions
     }
 }
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/RootDetector.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/RootDetector.kt
@@ -14,7 +14,7 @@ import java.util.concurrent.atomic.AtomicBoolean
  * possible to manipulate Java return values & native library loading, it will always be possible
  * for a determined application to defeat these root checks.
  */
-internal class RootDetector @JvmOverloads constructor(
+internal open class RootDetector @JvmOverloads constructor(
     private val deviceBuildInfo: DeviceBuildInfo = DeviceBuildInfo.defaultInfo(),
     private val rootBinaryLocations: List<String> = ROOT_INDICATORS,
     private val buildProps: File = BUILD_PROP_FILE,