diff --git a/app/src/desktopMain/kotlin/com/crosspaste/ui/paste/side/preview/SkiaAnimatedImageView.kt b/app/src/desktopMain/kotlin/com/crosspaste/ui/paste/side/preview/SkiaAnimatedImageView.kt index c0bb20b7a..a27b075e3 100644 --- a/app/src/desktopMain/kotlin/com/crosspaste/ui/paste/side/preview/SkiaAnimatedImageView.kt +++ b/app/src/desktopMain/kotlin/com/crosspaste/ui/paste/side/preview/SkiaAnimatedImageView.kt @@ -55,6 +55,92 @@ fun Path.isAnimatedImage(): Boolean = extension.lowercase() in animatedImageExte internal fun frameDurationMs(rawDurationMs: Int?): Int = (rawDurationMs ?: MIN_FRAME_DURATION_MS).coerceAtLeast(MIN_FRAME_DURATION_MS) +// Abstraction over Skia's Codec so [runAnimationLoop] can be exercised in tests +// without a real GIF on disk. Production wires it to [SkiaCodecDecoder]. +// +// Contract: decodeFrame must NOT throw — return false on failure. Implementations +// own any native pixel buffers and release them in [close]. +internal interface FrameDecoder { + val frameCount: Int + val srcSize: Size + + fun frameDelayMs(index: Int): Int + + fun newBuffer(): FrameBuffer + + suspend fun decodeFrame( + into: FrameBuffer, + index: Int, + ): Boolean + + fun close() +} + +internal interface FrameBuffer { + fun asImageBitmap(): ImageBitmap + + fun close() +} + +// Regression guard: see commit bb9a3819. readPixels is a synchronous JNI call +// that ignores coroutine cancellation. close() on the codec or any bitmap must +// run only AFTER an in-flight decodeFrame returns, otherwise the JNI call frees +// pixel buffers under its own feet and crashes the process. +// +// This is enforced structurally: +// - decoder + buffers are owned by THIS suspend function +// - finally blocks run after the last suspension point returns, so +// cancellation cannot race with an active decode +// +// Do NOT split decoder/buffer ownership across a sibling DisposableEffect or +// produceState.awaitDispose — those run concurrently with this function's +// withContext(ioDispatcher) and reintroduce the UAF. +internal suspend fun runAnimationLoop( + decoder: FrameDecoder, + isPlaying: () -> Boolean, + awaitPlaying: suspend () -> Unit, + onReady: (ImageBitmap, Size) -> Unit, +) { + try { + val bitmaps = Array(2) { decoder.newBuffer() } + try { + // Prime frame 0 before publishing Ready so the first paint is the + // real first frame, not an empty bitmap. + decoder.decodeFrame(bitmaps[0], 0) + + var frameIndex = 0 + var frontIndex = 0 + onReady(bitmaps[frontIndex].asImageBitmap(), decoder.srcSize) + + if (decoder.frameCount <= 1) awaitCancellation() + + while (true) { + if (!isPlaying()) { + // Suspend until isPlaying flips back to true instead of + // polling, so a hidden preview costs no wake-ups. + awaitPlaying() + continue + } + val durationMs = decoder.frameDelayMs(frameIndex) + delay(durationMs.milliseconds) + if (!isPlaying()) continue + val nextIndex = (frameIndex + 1) % decoder.frameCount + val backIndex = 1 - frontIndex + val success = decoder.decodeFrame(bitmaps[backIndex], nextIndex) + if (success) { + frameIndex = nextIndex + frontIndex = backIndex + onReady(bitmaps[frontIndex].asImageBitmap(), decoder.srcSize) + } + } + } finally { + bitmaps.forEach { it.close() } + } + } finally { + decoder.close() + } +} + private sealed interface FrameState { data object Loading : FrameState @@ -77,80 +163,25 @@ fun SkiaAnimatedImageView( var frameState by remember(path) { mutableStateOf(FrameState.Loading) } val playing by rememberUpdatedState(isPlaying) - // Codec + double-buffered bitmaps are owned by this single LaunchedEffect - // so the finally block runs only after any in-flight readPixels returns. - // readPixels is a synchronous JNI call that ignores coroutine cancellation, - // so releasing native resources from a sibling DisposableEffect.onDispose - // (or produceState's awaitDispose) could free pixel buffers under an - // active decode and crash with SIGSEGV. LaunchedEffect(path) { - val codec = + val decoder = withContext(ioDispatcher) { - runCatching { - val bytes = path.toFile().readBytes() - Codec.makeFromData(Data.makeFromBytes(bytes)) - }.onFailure { - logger.warn(it) { "Failed to decode animated image: $path" } - }.getOrNull() + runCatching { SkiaCodecDecoder.load(path) } + .onFailure { logger.warn(it) { "Failed to decode animated image: $path" } } + .getOrNull() } - if (codec == null) { + if (decoder == null) { frameState = FrameState.Failed return@LaunchedEffect } - try { - val bitmaps = Array(2) { Bitmap().apply { allocPixels(codec.imageInfo) } } - try { - val frameInfos = codec.framesInfo - val frameCount = codec.frameCount.coerceAtLeast(1) - val srcSize = Size(codec.imageInfo.width.toFloat(), codec.imageInfo.height.toFloat()) - - // Prime frame 0 before publishing Ready so the first paint is the - // real first frame, not an empty bitmap. - withContext(ioDispatcher) { - runCatching { codec.readPixels(bitmaps[0], 0) } - .onFailure { logger.warn(it) { "Failed to decode initial frame" } } - } - - var frameIndex = 0 - var frontIndex = 0 - frameState = FrameState.Ready(bitmaps[frontIndex].asComposeImageBitmap(), srcSize) - - if (frameCount <= 1) awaitCancellation() - - while (true) { - if (!playing) { - // Suspend until isPlaying flips back to true instead of - // polling, so a hidden preview costs no wake-ups. - snapshotFlow { playing }.first { it } - continue - } - val durationMs = frameDurationMs(frameInfos.getOrNull(frameIndex)?.duration) - delay(durationMs.milliseconds) - if (!playing) continue - val nextIndex = (frameIndex + 1) % frameCount - val backIndex = 1 - frontIndex - // Decode off the UI thread; LZW decompression on large frames - // can otherwise eat the next render window. - val success = - withContext(ioDispatcher) { - runCatching { codec.readPixels(bitmaps[backIndex], nextIndex) } - .onFailure { logger.warn(it) { "Failed to decode frame $nextIndex" } } - .isSuccess - } - if (success) { - frameIndex = nextIndex - frontIndex = backIndex - frameState = FrameState.Ready(bitmaps[frontIndex].asComposeImageBitmap(), srcSize) - } - } - } finally { - bitmaps.forEach { it.close() } - } - } finally { - codec.close() - } + runAnimationLoop( + decoder = decoder, + isPlaying = { playing }, + awaitPlaying = { snapshotFlow { playing }.first { it } }, + onReady = { bitmap, size -> frameState = FrameState.Ready(bitmap, size) }, + ) } when (val current = frameState) { @@ -175,6 +206,49 @@ fun SkiaAnimatedImageView( } } +private class SkiaCodecDecoder( + private val codec: Codec, +) : FrameDecoder { + private val framesInfo = codec.framesInfo + + override val frameCount: Int = codec.frameCount.coerceAtLeast(1) + + override val srcSize: Size = + Size(codec.imageInfo.width.toFloat(), codec.imageInfo.height.toFloat()) + + override fun frameDelayMs(index: Int): Int = frameDurationMs(framesInfo.getOrNull(index)?.duration) + + override fun newBuffer(): FrameBuffer = SkiaBitmapBuffer(Bitmap().apply { allocPixels(codec.imageInfo) }) + + override suspend fun decodeFrame( + into: FrameBuffer, + index: Int, + ): Boolean = + withContext(ioDispatcher) { + require(into is SkiaBitmapBuffer) { "Expected SkiaBitmapBuffer, got ${into::class}" } + runCatching { codec.readPixels(into.bitmap, index) } + .onFailure { logger.warn(it) { "Failed to decode frame $index" } } + .isSuccess + } + + override fun close() = codec.close() + + companion object { + fun load(path: Path): SkiaCodecDecoder { + val bytes = path.toFile().readBytes() + return SkiaCodecDecoder(Codec.makeFromData(Data.makeFromBytes(bytes))) + } + } +} + +private class SkiaBitmapBuffer( + val bitmap: Bitmap, +) : FrameBuffer { + override fun asImageBitmap(): ImageBitmap = bitmap.asComposeImageBitmap() + + override fun close() = bitmap.close() +} + @Composable private fun LoadingIndicator() { Row( diff --git a/app/src/desktopTest/kotlin/com/crosspaste/ui/paste/side/preview/SkiaAnimatedImageViewTest.kt b/app/src/desktopTest/kotlin/com/crosspaste/ui/paste/side/preview/SkiaAnimatedImageViewTest.kt index 43f27f017..a55fe39d5 100644 --- a/app/src/desktopTest/kotlin/com/crosspaste/ui/paste/side/preview/SkiaAnimatedImageViewTest.kt +++ b/app/src/desktopTest/kotlin/com/crosspaste/ui/paste/side/preview/SkiaAnimatedImageViewTest.kt @@ -1,5 +1,17 @@ package com.crosspaste.ui.paste.side.preview +import androidx.compose.ui.geometry.Size +import androidx.compose.ui.graphics.ImageBitmap +import io.mockk.mockk +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.withContext +import kotlinx.coroutines.yield import okio.Path.Companion.toPath import kotlin.test.Test import kotlin.test.assertEquals @@ -93,4 +105,254 @@ class SkiaAnimatedImageViewTest { assertEquals(100, frameDurationMs(100)) assertEquals(1_000, frameDurationMs(1_000)) } + + // --- runAnimationLoop close-ordering regression (commit bb9a3819) --- + // + // The production bug: codec.readPixels is a synchronous JNI call that + // ignores coroutine cancellation. When buffer/codec close used to live in + // a sibling DisposableEffect.onDispose or produceState.awaitDispose, those + // could free pixel buffers while readPixels was still touching them + // (SIGSEGV). bb9a3819 folded ownership into the LaunchedEffect so the + // finally blocks run only after the in-flight decode returns. + // + // The fake decoder simulates JNI behavior with withContext(NonCancellable) + // around a CompletableDeferred — cancellation cannot pre-empt it. + + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun `runAnimationLoop close-ordering survives cancel during in-flight decode`() = + runTest(UnconfinedTestDispatcher()) { + val events = mutableListOf() + val decodeStarted = CompletableDeferred() + val decodeUnblock = CompletableDeferred() + val decoder = + FakeFrameDecoder( + events = events, + frameCount = 4, + blockingFrames = + mapOf( + 0 to BlockingFrame(decodeStarted, decodeUnblock), + ), + ) + + val job = + launch { + runAnimationLoop( + decoder = decoder, + isPlaying = { true }, + awaitPlaying = { }, + onReady = { _, _ -> }, + ) + } + + // Frame 0 priming entered the simulated JNI section. + decodeStarted.await() + assertTrue( + "decode.start(0)" in events, + "decode start was not recorded: $events", + ) + + // Request cancellation while the decode is still in flight. The + // structural guarantee under test: nothing close()s yet. + job.cancel() + yield() + yield() + assertFalse( + "decode.end(0)" in events, + "decode returned before we unblocked it; test setup broken: $events", + ) + assertFalse( + "buffer.close" in events, + "buffer.close ran during in-flight decode — UAF reintroduced (events=$events)", + ) + assertFalse( + "decoder.close" in events, + "decoder.close ran during in-flight decode — UAF reintroduced (events=$events)", + ) + + // Release the simulated JNI call; finally blocks should now run. + decodeUnblock.complete(Unit) + job.join() + + val decodeEndIdx = events.indexOf("decode.end(0)") + assertTrue(decodeEndIdx >= 0, "decode.end(0) never observed: $events") + events.forEachIndexed { i, event -> + if (event == "buffer.close" || event == "decoder.close") { + assertTrue( + i > decodeEndIdx, + "$event at $i must follow decode.end(0) at $decodeEndIdx (events=$events)", + ) + } + } + + // Both back/front buffers closed exactly once. + assertEquals( + 2, + events.count { it == "buffer.close" }, + "expected both bitmap buffers closed (events=$events)", + ) + // Decoder closes exactly once, AND last — the outer finally runs + // after the inner finally that closes the bitmaps. + assertEquals( + 1, + events.count { it == "decoder.close" }, + "expected decoder closed once (events=$events)", + ) + assertEquals( + "decoder.close", + events.last(), + "decoder.close must be the final event so it follows bitmap close (events=$events)", + ) + } + + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun `runAnimationLoop closes resources cleanly on normal cancel`() = + runTest(UnconfinedTestDispatcher()) { + val events = mutableListOf() + // frameCount == 1 → loop awaitCancellation()s after priming + val decoder = FakeFrameDecoder(events = events, frameCount = 1) + var readyCount = 0 + + val job = + launch { + runAnimationLoop( + decoder = decoder, + isPlaying = { true }, + awaitPlaying = { }, + onReady = { _, _ -> readyCount++ }, + ) + } + + // Priming decode + onReady run eagerly under UnconfinedTestDispatcher + // before the loop reaches awaitCancellation. + yield() + + job.cancelAndJoin() + + assertEquals(1, readyCount, "single-frame path should emit onReady once") + assertEquals(2, events.count { it == "buffer.close" }) + assertEquals(1, events.count { it == "decoder.close" }) + assertEquals( + "decoder.close", + events.last(), + "decoder.close must run after both bitmap closes (events=$events)", + ) + } + + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun `runAnimationLoop close-ordering survives cancel during mid-loop decode`() = + runTest(UnconfinedTestDispatcher()) { + val events = mutableListOf() + val decode1Started = CompletableDeferred() + val decode1Unblock = CompletableDeferred() + val decoder = + FakeFrameDecoder( + events = events, + frameCount = 3, + blockingFrames = + mapOf( + // Frame 1 is the first frame decoded inside the while loop; + // priming uses frame 0 which we let return immediately. + 1 to BlockingFrame(decode1Started, decode1Unblock), + ), + ) + + val job = + launch { + runAnimationLoop( + decoder = decoder, + isPlaying = { true }, + awaitPlaying = { }, + onReady = { _, _ -> }, + ) + } + + // Advance virtual time so the priming decode finishes, the loop + // delays MIN_FRAME_DURATION_MS, then enters decodeFrame(_, 1). + decode1Started.await() + job.cancel() + yield() + yield() + assertFalse( + "buffer.close" in events, + "buffer.close fired mid-decode (events=$events)", + ) + assertFalse( + "decoder.close" in events, + "decoder.close fired mid-decode (events=$events)", + ) + + decode1Unblock.complete(Unit) + job.join() + + val decode1EndIdx = events.indexOf("decode.end(1)") + assertTrue(decode1EndIdx >= 0, "decode.end(1) never observed: $events") + events.forEachIndexed { i, event -> + if (event == "buffer.close" || event == "decoder.close") { + assertTrue( + i > decode1EndIdx, + "$event at $i must follow decode.end(1) at $decode1EndIdx (events=$events)", + ) + } + } + assertEquals( + "decoder.close", + events.last(), + "decoder.close must be the final event (events=$events)", + ) + } +} + +private data class BlockingFrame( + val started: CompletableDeferred, + val unblock: CompletableDeferred, +) + +private class FakeFrameDecoder( + private val events: MutableList, + override val frameCount: Int, + private val blockingFrames: Map = emptyMap(), +) : FrameDecoder { + override val srcSize: Size = Size(1f, 1f) + + override fun frameDelayMs(index: Int): Int = MIN_FRAME_DURATION_MS + + override fun newBuffer(): FrameBuffer = FakeFrameBuffer(events) + + override suspend fun decodeFrame( + into: FrameBuffer, + index: Int, + ): Boolean { + events += "decode.start($index)" + blockingFrames[index]?.let { hook -> + // Simulate readPixels: a synchronous JNI call that ignores coroutine + // cancellation. NonCancellable prevents the await() from being + // pre-empted, reproducing the "must finish before cancellation + // propagates" property that the bb9a3819 fix relies on. + withContext(NonCancellable) { + hook.started.complete(Unit) + hook.unblock.await() + } + } + events += "decode.end($index)" + return true + } + + override fun close() { + events += "decoder.close" + } +} + +private class FakeFrameBuffer( + private val events: MutableList, +) : FrameBuffer { + private val image: ImageBitmap = mockk(relaxed = true) + + override fun asImageBitmap(): ImageBitmap = image + + override fun close() { + events += "buffer.close" + } }