Merge pull request #16 from appdevforall/fix/phase1-security-d12-process-drain

fix(controller): drain process output to avoid deadlock/silent failures (D12)

Author: luisguzman-adfa

controller/app/src/main/java/org/iiab/controller/DeployFragment.java

@@ -58,6 +58,7 @@
 import org.iiab.controller.rootfs.presentation.RootfsViewModel;
 import org.iiab.controller.rootfs.presentation.RootfsViewModelFactory;
 import org.iiab.controller.util.ByteFormatter;
+import org.iiab.controller.util.ProcessRunner;
 
 import org.json.JSONObject;
 
@@ -1086,8 +1087,12 @@ public void onComplete(String destDir) {
                                 File binDir = new File(requireContext().getFilesDir(), "usr/bin");
                                 if (binDir.exists()) {
                                     try {
-                                        Runtime.getRuntime().exec(new String[]{"chmod", "-R", "755", binDir.getAbsolutePath()}).waitFor();
-                                    } catch (Exception ignored) {
+                                        ProcessRunner.Result chmodResult = ProcessRunner.run(new String[]{"chmod", "-R", "755", binDir.getAbsolutePath()});
+                                        if (!chmodResult.isSuccess()) {
+                                            Log.w(TAG, "chmod on usr/bin failed (exit " + chmodResult.exitCode + "): " + chmodResult.output);
+                                        }
+                                    } catch (Exception e) {
+                                        Log.w(TAG, "chmod on usr/bin failed", e);
                                     }
                                 }
 
@@ -1137,8 +1142,12 @@ public void onError(String error) {
                             btnFastInstall.setEnabled(false);
                             new Thread(() -> {
                                 try {
-                                    Runtime.getRuntime().exec(new String[]{"rm", "-rf", debianRootfs.getAbsolutePath()}).waitFor();
-                                } catch (Exception ignored) {
+                                    ProcessRunner.Result wipeResult = ProcessRunner.run(new String[]{"rm", "-rf", debianRootfs.getAbsolutePath()});
+                                    if (!wipeResult.isSuccess()) {
+                                        Log.w(TAG, "rm -rf rootfs (reinstall) failed (exit " + wipeResult.exitCode + "): " + wipeResult.output);
+                                    }
+                                } catch (Exception e) {
+                                    Log.w(TAG, "rm -rf rootfs (reinstall) failed", e);
                                 }
                                 mainAct.runOnUiThread(executeDownload);
                             }).start();
@@ -1175,8 +1184,10 @@ private void bindDeleteButtonLogic(MainActivity mainAct, File debianRootfs) {
                         new Thread(() -> {
                             enableSystemProtection();
                             try {
-                                Process p = Runtime.getRuntime().exec(new String[]{"rm", "-rf", debianRootfs.getAbsolutePath()});
-                                p.waitFor();
+                                ProcessRunner.Result wipeResult = ProcessRunner.run(new String[]{"rm", "-rf", debianRootfs.getAbsolutePath()});
+                                if (!wipeResult.isSuccess()) {
+                                    Log.w(TAG, "rm -rf rootfs (delete) failed (exit " + wipeResult.exitCode + "): " + wipeResult.output);
+                                }
                             } catch (Exception e) {
                                 mainAct.runOnUiThread(() -> Snackbar.make(getView(), getString(R.string.install_error_delete, e.getMessage()), Snackbar.LENGTH_LONG).show());
                             } finally {
@@ -1227,7 +1238,10 @@ private void bindResetButtonLogic(MainActivity mainAct, File debianRootfs) {
                                 });
 
                                 // 1. WIPE
-                                Runtime.getRuntime().exec(new String[]{"rm", "-rf", debianRootfs.getAbsolutePath()}).waitFor();
+                                ProcessRunner.Result wipeResult = ProcessRunner.run(new String[]{"rm", "-rf", debianRootfs.getAbsolutePath()});
+                                if (!wipeResult.isSuccess()) {
+                                    Log.w(TAG, "rm -rf rootfs (vanilla reset) failed (exit " + wipeResult.exitCode + "): " + wipeResult.output);
+                                }
                                 debianRootfs.mkdirs();
 
                                 // 2. DOWNLOAD
@@ -1880,8 +1894,13 @@ private void bindBackupButtonLogic(MainActivity mainAct, File backupsDir, File i
                     String gzipBin = staticGzip.exists() ? staticGzip.getAbsolutePath() : "gzip";
 
                     String cmd = tarBin + " -cf - -C " + iiabRootDir.getAbsolutePath() + " installed-rootfs | " + gzipBin + " > " + backupFile.getAbsolutePath();
-                    Process p = Runtime.getRuntime().exec(new String[]{"/system/bin/sh", "-c", cmd});
-                    int exitCode = p.waitFor();
+                    // D12: ProcessRunner drains stderr so a large backup with tar warnings
+                    // cannot deadlock on a full pipe buffer.
+                    ProcessRunner.Result backupResult = ProcessRunner.run(new String[]{"/system/bin/sh", "-c", cmd});
+                    int exitCode = backupResult.exitCode;
+                    if (exitCode != 0) {
+                        Log.w(TAG, "Backup pipe failed (exit " + exitCode + "): " + backupResult.output);
+                    }
 
                     mainAct.runOnUiThread(() -> {
                         if (isBackupInProgress) {

controller/app/src/main/java/org/iiab/controller/util/ProcessRunner.java

@@ -0,0 +1,73 @@
+/*
+ * ============================================================================
+ * Name        : ProcessRunner.java
+ * Author      : IIAB Project
+ * Copyright   : Copyright (c) 2026 IIAB Project
+ * Description : Small helper to run an external process safely: it always
+ *               drains the output so the child cannot deadlock on a full pipe
+ *               buffer, and returns the exit code instead of letting callers
+ *               swallow failures. Addresses tech-debt item D12.
+ * ============================================================================
+ */
+package org.iiab.controller.util;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+
+/**
+ * Runs an external command and <strong>always reads its output</strong>.
+ *
+ * <p>Calling {@code Runtime.exec(...).waitFor()} without reading the child's
+ * stdout/stderr risks a deadlock: once the OS pipe buffer (~64&nbsp;KB) fills,
+ * the child blocks writing while the parent blocks in {@code waitFor()} — see
+ * tech-debt item <b>D12</b>. This helper merges stderr into stdout
+ * ({@link ProcessBuilder#redirectErrorStream(boolean)}) so a single read drains
+ * everything, then returns the exit code and captured output so callers can log
+ * or react to failures instead of ignoring them.
+ *
+ * <p>Process I/O only (no {@code android.*}); call it off the main thread.
+ */
+public final class ProcessRunner {
+
+    /** Outcome of a process run: its exit code and combined stdout+stderr. */
+    public static final class Result {
+        public final int exitCode;
+        public final String output;
+
+        Result(int exitCode, String output) {
+            this.exitCode = exitCode;
+            this.output = output;
+        }
+
+        public boolean isSuccess() {
+            return exitCode == 0;
+        }
+    }
+
+    private ProcessRunner() {
+        // Static utility; not instantiable.
+    }
+
+    /**
+     * Start {@code command}, drain its merged stdout+stderr to completion, wait
+     * for it to exit, and return the exit code together with the captured output.
+     */
+    public static Result run(String[] command) throws IOException, InterruptedException {
+        ProcessBuilder pb = new ProcessBuilder(command);
+        pb.redirectErrorStream(true); // merge stderr into stdout: one drain can't deadlock
+        Process process = pb.start();
+
+        StringBuilder out = new StringBuilder();
+        try (BufferedReader reader =
+                     new BufferedReader(new InputStreamReader(process.getInputStream()))) {
+            String line;
+            while ((line = reader.readLine()) != null) {
+                out.append(line).append('\n');
+            }
+        }
+
+        int exitCode = process.waitFor();
+        return new Result(exitCode, out.toString());
+    }
+}

controller/docs/TECH_DEBT_PLAN.md

@@ -52,7 +52,12 @@ _Last updated: 2026-06-17. Tracks remediation work against the findings below. I
 - New pure domain rule `org.iiab.controller.deploy.domain.ModuleName.isAllowed(name, known)`: the name must be a known catalog key (allowlist) AND match `[a-z0-9_-]` (no quotes/`;`/`&&`/`$()`). `ModuleRegistry.validYamlKeys()` is the single allowlist source. Unit-tested (`ModuleNameTest`).
 - The guard fails closed: an unrecognized/unsafe module is logged and skipped before the command is built; the command structure is unchanged for legitimate modules. Lower-risk on-device `sh -c` pipes (extract/backup, app-internal paths) are a documented follow-up.
 
-**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3** (PR #10), **D6** (PR #12), **D2**. Remaining: **D12**, **S4**, **D11**.
+**D12 — Undrained process pipes / swallowed exec errors (Phase 1 security): DONE** (PR `fix/phase1-security-d12-process-drain`)
+- Closes **D12**: several `Runtime.exec(...).waitFor()` calls in `DeployFragment` never read the child's output (deadlock risk once the ~64 KB pipe buffer fills — notably the backup `tar | gzip` pipe on a large rootfs) and some swallowed failures in empty `catch (Exception ignored) {}`.
+- New shared `util/ProcessRunner.run(cmd)`: `redirectErrorStream(true)` + full drain + returns `{exitCode, output}`, so a single read cannot deadlock and callers can log/handle failures.
+- Migrated the raw `exec().waitFor()` sites (backup, `chmod -R`, the three `rm -rf` wipes); empty catches now log. Left the extraction path (already drains stderr) and the `getprop` read (reads stdout) as-is. No new unit test (process glue, not pure logic — fragile to run a shell in unit tests on a Windows dev box); verified by inspection + CI compile.
+
+**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3** (PR #10), **D6** (PR #12), **D2** (PR #13), **D12**. Remaining: **D11**, **S4**, **F15**.
 ## 1. Executive summary
 
 The Controller is functional and shows real security intent (it SHA256-audits native binaries at build time, scrubs the keystore in CI, and scopes most broadcasts). But it carries debt on four fronts that scale badly toward the README's "millions of users" goal: