Skip to content

Commit 2d9065b

Browse files
committed
fix: avoid potential hang as it blocks on process.getInputStream().readAllBytes() before timeout checking
Signed-off-by: Chao Wang <chaowan@redhat.com>
1 parent 025fdf2 commit 2d9065b

1 file changed

Lines changed: 64 additions & 21 deletions

File tree

src/main/java/io/github/guacsec/trustifyda/providers/CargoProvider.java

Lines changed: 64 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,12 @@
4545
import java.util.HashSet;
4646
import java.util.Map;
4747
import java.util.Set;
48+
import java.util.concurrent.ExecutionException;
49+
import java.util.concurrent.ExecutorService;
50+
import java.util.concurrent.Executors;
51+
import java.util.concurrent.Future;
4852
import java.util.concurrent.TimeUnit;
53+
import java.util.concurrent.TimeoutException;
4954
import java.util.logging.Logger;
5055
import org.tomlj.Toml;
5156
import org.tomlj.TomlParseResult;
@@ -341,23 +346,64 @@ private CargoMetadata executeCargoMetadata() throws IOException, InterruptedExce
341346
.directory(workingDir.toFile())
342347
.start();
343348

349+
// Use bounded executor to read streams concurrently to avoid two potential deadlocks:
350+
// 1. buffer deadlock (process blocks on write when output buffers fill up while Java waits for
351+
// process completion)
352+
// 2. stalled process deadlock (readAllBytes() hangs forever when cargo process stalls
353+
// completely with no timeout protection)
354+
// Bounded executor allows proper cancellation and cleanup vs CompletableFuture common pool
355+
ExecutorService streamExecutor = Executors.newFixedThreadPool(2);
344356
String output;
345-
try (InputStream is = process.getInputStream()) {
346-
output = new String(is.readAllBytes(), StandardCharsets.UTF_8);
347-
}
357+
String errorOutput;
358+
359+
try {
360+
Future<String> outputFuture =
361+
streamExecutor.submit(
362+
() -> {
363+
try (InputStream is = process.getInputStream()) {
364+
return new String(is.readAllBytes(), StandardCharsets.UTF_8);
365+
} catch (IOException e) {
366+
log.warning("Failed to read stdout from cargo metadata: " + e.getMessage());
367+
return "";
368+
}
369+
});
370+
371+
Future<String> errorFuture =
372+
streamExecutor.submit(
373+
() -> {
374+
try (InputStream is = process.getErrorStream()) {
375+
return new String(is.readAllBytes(), StandardCharsets.UTF_8);
376+
} catch (IOException e) {
377+
log.warning("Failed to read stderr from cargo metadata: " + e.getMessage());
378+
return "";
379+
}
380+
});
381+
382+
boolean finished = process.waitFor(TIMEOUT, TimeUnit.SECONDS);
383+
384+
if (!finished) {
385+
process.destroyForcibly();
386+
outputFuture.cancel(true);
387+
errorFuture.cancel(true);
388+
throw new InterruptedException("cargo metadata timed out after " + TIMEOUT + " seconds");
389+
}
348390

349-
boolean finished = process.waitFor(TIMEOUT, TimeUnit.SECONDS);
391+
try {
392+
// Short timeout since process already finished
393+
output = outputFuture.get(1, TimeUnit.SECONDS);
394+
errorOutput = errorFuture.get(1, TimeUnit.SECONDS);
395+
} catch (ExecutionException | TimeoutException e) {
396+
log.warning("Failed to read process output: " + e.getMessage());
397+
return null;
398+
}
399+
} finally {
400+
streamExecutor.shutdownNow();
401+
}
350402

403+
// Safe to call exitValue() - we confirmed the process finished
351404
int exitCode = process.exitValue();
352405

353406
if (exitCode != 0) {
354-
String errorOutput = "";
355-
try (InputStream errorStream = process.getErrorStream()) {
356-
errorOutput = new String(errorStream.readAllBytes(), StandardCharsets.UTF_8);
357-
} catch (IOException e) {
358-
log.warning("Failed to read error stream: " + e.getMessage());
359-
}
360-
361407
String errorMessage = "cargo metadata failed with exit code: " + exitCode;
362408
if (!errorOutput.isEmpty()) {
363409
errorMessage += ". Error: " + errorOutput.trim();
@@ -373,11 +419,6 @@ private CargoMetadata executeCargoMetadata() throws IOException, InterruptedExce
373419
return null;
374420
}
375421

376-
if (!finished) {
377-
process.destroyForcibly();
378-
throw new InterruptedException("cargo metadata timed out after " + TIMEOUT + " seconds");
379-
}
380-
381422
try {
382423
CargoMetadata metadata = MAPPER.readValue(output, CargoMetadata.class);
383424
if (debugLoggingIsNeeded()) {
@@ -552,12 +593,14 @@ private DependencyInfo getPackageInfo(String packageId, Map<String, CargoPackage
552593
public CargoProvider(Path manifest) {
553594
super(Type.CARGO, manifest);
554595
this.cargoExecutable = Operations.getExecutable("cargo", "--version");
555-
if (cargoExecutable != null) {
556-
log.info("Found cargo executable: " + cargoExecutable);
557-
} else {
558-
log.warning("Cargo executable not found - dependency analysis will not work");
596+
if (debugLoggingIsNeeded()) {
597+
if (cargoExecutable != null) {
598+
log.info("Found cargo executable: " + cargoExecutable);
599+
} else {
600+
log.warning("Cargo executable not found - dependency analysis will not work");
601+
}
602+
log.info("Initialized RustProvider for manifest: " + manifest);
559603
}
560-
log.info("Initialized RustProvider for manifest: " + manifest);
561604
}
562605

563606
@Override

0 commit comments

Comments
 (0)