From 847c79ea9a9422721e2db92323b382d68634158d Mon Sep 17 00:00:00 2001 From: Bryan Chan Date: Fri, 19 Jun 2026 05:02:53 -0700 Subject: [PATCH 1/2] ADFA-3945: extract tar.xz via redirected output file (removes reader-thread race) The Termux tar extraction drained the child's stdout on a separate reader thread. On a 2-minute timeout the code calls destroyForcibly(), which closes the pipe; the reader thread's in-flight read() then throws InterruptedIOException on a bare thread with no handler, crashing the app (Sentry APPDEVFORALL-V0). Fix (root cause, not the symptom): redirect the child's combined stdout/stderr to a temp file via ProcessBuilder.redirectOutput(), drop the reader thread entirely, and read the log back after waitFor(). With OS-side redirection there is no concurrent read to interrupt when the pipe is closed, and the process also can't deadlock on a full pipe buffer. destroyForcibly() on timeout is retained; the temp log is deleted after use. This eliminates the entire bug class rather than catching the exception, and makes a future timeout/cancellation change safe (no race to reintroduce). Per Hal's suggestion on ADFA-3945. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../manager/services/IdeArchiveServiceImpl.kt | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt index 36b3b47230..174b8e265c 100644 --- a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt +++ b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt @@ -18,7 +18,6 @@ import java.io.FilterInputStream import java.io.InputStream import java.io.OutputStream import java.util.concurrent.TimeUnit -import kotlin.concurrent.thread import kotlin.system.measureTimeMillis class IdeArchiveServiceImpl( @@ -255,6 +254,21 @@ class IdeArchiveServiceImpl( } } + /** + * Extracts a `.tar.xz` archive by invoking Termux `tar`. + * + * The child process's combined stdout/stderr is redirected to a temporary log file + * ([ProcessBuilder.redirectOutput]) and read back after the process exits, rather than being + * drained by a separate reader thread. This removes the concurrency hazard behind ADFA-3945 + * (Sentry APPDEVFORALL-V0): when a timeout triggers [Process.destroyForcibly] the pipe is + * closed, and a thread blocked in `read()` on it would throw an `InterruptedIOException` on a + * bare thread with no handler, crashing the app. With OS-side redirection there is no in-flight + * read to interrupt, and the process also cannot deadlock on a full pipe buffer. + * + * @param archiveFile the `.tar.xz` to extract. + * @param outputDir the directory to extract into (must exist and be writable). + * @return `true` only if `tar` completed within the timeout with exit code 0. + */ private fun extractTarXzViaTermux(archiveFile: File, outputDir: File): Boolean { if (!archiveFile.exists()) { logger.debug("Archive not found: ${archiveFile.absolutePath}") @@ -263,28 +277,25 @@ class IdeArchiveServiceImpl( logger.debug("Starting Termux tar extraction: ${archiveFile.absolutePath}") + val logFile = File.createTempFile("tarxz-extract", ".log", outputDir) return runCatching { - val output = StringBuilder() var exitCode = -1 val elapsed = measureTimeMillis { val process = ProcessBuilder( "$TERMUX_BIN_PATH/tar", "-xJf", archiveFile.absolutePath, "-C", outputDir.canonicalPath, "--no-same-owner" - ).redirectErrorStream(true).apply { - environment()["PATH"] = TERMUX_BIN_PATH - }.start() - - val reader = thread(name = "tar-xz-extract-output") { - process.inputStream.bufferedReader().useLines { it.forEach(output::appendLine) } - } + ).redirectErrorStream(true) + .redirectOutput(logFile) + .apply { environment()["PATH"] = TERMUX_BIN_PATH } + .start() val completed = process.waitFor(2, TimeUnit.MINUTES) if (!completed) process.destroyForcibly() - reader.join() exitCode = if (completed) process.exitValue() else -1 } + val output = runCatching { logFile.readText() }.getOrDefault("") when (exitCode) { 0 -> { logger.debug("Extraction succeeded in ${elapsed}ms: $output") @@ -298,6 +309,8 @@ class IdeArchiveServiceImpl( }.getOrElse { e -> logger.error("Termux process error: ${e.message}") false + }.also { + logFile.delete() } } From cb178fa02656c012ac5ab73934350b60ebabd2ec Mon Sep 17 00:00:00 2001 From: Bryan Chan Date: Thu, 2 Jul 2026 15:53:11 -0700 Subject: [PATCH 2/2] ADFA-3945: harden tar-timeout process cleanup (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two CodeRabbit findings on the tar.xz extraction path. 1. Process lifecycle around waitFor(2, MINUTES): destroyForcibly() is asynchronous and an InterruptedException from waitFor left the tar child running. - On timeout: destroyForcibly() THEN a bounded waitFor(10s) so the kill actually lands before we read the exit code and the redirected log — the process (and its output pipe) can no longer outlive this method. - On InterruptedException from the primary wait: destroyForcibly(), restore the interrupt flag (Thread.currentThread().interrupt()), and rethrow. The runCatching result is unwrapped via getOrElse, which re-throws InterruptedException so the cancellation genuinely unwinds the stack instead of being downgraded to a plain `false` return (a caller inspecting only the boolean would otherwise mistake a cancellation for an ordinary extraction failure). The bounded cleanup wait also restores the flag if interrupted. 2. The temp extraction-log cleanup ignored logFile.delete()'s return — now checked and warned via the existing logger (matching the two sibling delete-warn sites above). Cleanup runs unconditionally before the result is unwrapped, so it still happens on the interruption-propagation path. Verified: :plugin-manager:compileV8DebugKotlin on the canonical flox jdk17 — BUILD SUCCESSFUL. The extraction method shells out to Termux `tar`, so the timeout/interrupt path is on-device/integration-level and not host-unit-testable without injecting ProcessBuilder (out of scope for these findings); plugin-manager ships no test source set. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../manager/services/IdeArchiveServiceImpl.kt | 39 ++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt index 174b8e265c..b369d569d3 100644 --- a/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt +++ b/plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeArchiveServiceImpl.kt @@ -278,7 +278,7 @@ class IdeArchiveServiceImpl( logger.debug("Starting Termux tar extraction: ${archiveFile.absolutePath}") val logFile = File.createTempFile("tarxz-extract", ".log", outputDir) - return runCatching { + val result = runCatching { var exitCode = -1 val elapsed = measureTimeMillis { @@ -290,8 +290,26 @@ class IdeArchiveServiceImpl( .apply { environment()["PATH"] = TERMUX_BIN_PATH } .start() - val completed = process.waitFor(2, TimeUnit.MINUTES) - if (!completed) process.destroyForcibly() + val completed = try { + process.waitFor(2, TimeUnit.MINUTES) + } catch (e: InterruptedException) { + // Interrupted mid-wait: kill the child so it can't outlive us, restore the + // interrupt flag, and rethrow — surfaced as cancellation by getOrElse below. + process.destroyForcibly() + Thread.currentThread().interrupt() + throw e + } + if (!completed) { + // Timed out. destroyForcibly() is asynchronous, so wait (bounded) for the kill + // to actually land before we read the exit code and the log — otherwise the tar + // process (and its redirected output pipe) can outlive this method. + process.destroyForcibly() + try { + process.waitFor(10, TimeUnit.SECONDS) + } catch (e: InterruptedException) { + Thread.currentThread().interrupt() + } + } exitCode = if (completed) process.exitValue() else -1 } @@ -306,11 +324,20 @@ class IdeArchiveServiceImpl( false } } - }.getOrElse { e -> + } + + // Always clean up the temp log, regardless of how the block above resolved. `result` is a + // Result (runCatching never throws), so this runs before we unwrap it below. + if (!logFile.delete()) { + logger.warn("Failed to delete temp extraction log: ${logFile.absolutePath}") + } + + return result.getOrElse { e -> + // Let an interruption unwind the stack as cancellation instead of masquerading as a + // plain extraction failure; the interrupt flag was already restored at the throw site. + if (e is InterruptedException) throw e logger.error("Termux process error: ${e.message}") false - }.also { - logFile.delete() } }