Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,26 @@
// limitations under the License.
package com.google.devtools.build.lib.query2.query.output;

import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.packages.LabelPrinter;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.query2.engine.OutputFormatterCallback;
import com.google.devtools.build.lib.query2.proto.proto2api.Build;
import com.google.protobuf.CodedOutputStream;

import java.io.IOException;
import java.io.OutputStream;
import java.util.List;
import java.util.concurrent.*;
import java.util.concurrent.atomic.AtomicBoolean;

/**
* An output formatter that outputs a protocol buffer representation of a query result and outputs
* the proto bytes to the output print stream. By taking the bytes and calling {@code mergeFrom()}
* on a {@code Build.QueryResult} object the full result can be reconstructed.
*/
public class StreamedProtoOutputFormatter extends ProtoOutputFormatter {

@Override
public String getName() {
return "streamed_proto";
Expand All @@ -34,13 +42,120 @@ public String getName() {
public OutputFormatterCallback<Target> createPostFactoStreamCallback(
final OutputStream out, final QueryOptions options, LabelPrinter labelPrinter) {
return new OutputFormatterCallback<Target>() {
private static final int MAX_CHUNKS_IN_QUEUE = Runtime.getRuntime().availableProcessors() * 2;
Comment on lines 43 to +45
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested Anonymous Classes

Complex parallel processing logic is implemented within an anonymous inner class. This creates maintainability issues as the code is deeply nested and tightly coupled to its parent class. Extracting this to a named class would improve readability and testability.

Standards
  • Clean-Code-Class-Organization
  • Maintainability-Quality-Nesting
  • Clean-Code-Functions

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic Numbers Usage

The code uses a magic number (500) for chunk size without explanation of why this value was chosen. This reduces maintainability as future developers won't understand the reasoning behind this specific value or know when it should be adjusted.

Standards
  • Clean-Code-Naming
  • Maintainability-Quality-Constants
  • Clean-Code-Comments

private static final int TARGETS_PER_CHUNK = 500;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic Number Usage

Magic number 500 lacks explanation for its selection. This hinders maintainability as future developers cannot understand the rationale for this specific batch size value.

      // Balance between memory usage and parallelization efficiency
      // Experimentally determined optimal value for typical target sizes
      private static final int TARGETS_PER_CHUNK = 500;
Standards
  • Clean-Code-No-Magic-Numbers
  • Clean-Code-Comments


private final LabelPrinter ourLabelPrinter = labelPrinter;

@Override
public void processOutput(Iterable<Target> partialResult)
throws IOException, InterruptedException {
for (Target target : partialResult) {
toTargetProtoBuffer(target, labelPrinter).writeDelimitedTo(out);
ForkJoinTask<?> writeAllTargetsFuture;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource Management Issue: The ForkJoinPool is created with parallelism equal to all available processors without any safeguards against resource exhaustion. On many-core systems, this could lead to excessive thread creation and CPU contention.

Consider limiting the maximum parallelism and adding an exception handler:

// Limit the number of threads to avoid resource exhaustion
int parallelism = Math.min(Runtime.getRuntime().availableProcessors(), 4);
try (ForkJoinPool executor = new ForkJoinPool(
    parallelism,
    ForkJoinPool.defaultForkJoinWorkerThreadFactory,
    (thread, throwable) -> {
        // Log uncaught exceptions in worker threads
        System.err.println("Worker thread exception: " + throwable);
    },
    // we use asyncMode to ensure the queue is processed FIFO
    true)) {

try (ForkJoinPool executor =
new ForkJoinPool(
Runtime.getRuntime().availableProcessors(),
ForkJoinPool.defaultForkJoinWorkerThreadFactory,
null,
// we use asyncMode to ensure the queue is processed FIFO, which maximizes
// throughput
true)) {
Comment on lines +54 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclosed ForkJoinPool

The ForkJoinPool is created with try-with-resources but the Future's completion is awaited after the try block exits, risking premature pool shutdown before task completion.

ForkJoinPool executor = new ForkJoinPool(
    Runtime.getRuntime().availableProcessors(),
    ForkJoinPool.defaultForkJoinWorkerThreadFactory,
    null,
    // we use asyncMode to ensure the queue is processed FIFO, which maximizes
    // throughput
    true);
try {
  var targetQueue = new LinkedBlockingQueue<Future<List<byte[]>>>(MAX_CHUNKS_IN_QUEUE);
  var stillAddingTargetsToQueue = new AtomicBoolean(true);
  // Rest of the code...
  // After all processing is complete:
  writeAllTargetsFuture.get();
} finally {
  executor.shutdown();
}
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness

Comment on lines +54 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread Pool Lifecycle

Creating a new ForkJoinPool for each processOutput call is expensive. Each pool creation allocates thread resources that must be initialized and later destroyed. Under high query volume, this pattern creates significant thread churn and initialization overhead, potentially degrading overall system performance.

// Use a shared thread pool to avoid creation/destruction overhead
private static final ForkJoinPool SHARED_EXECUTOR = new ForkJoinPool(
    Runtime.getRuntime().availableProcessors(),
    ForkJoinPool.defaultForkJoinWorkerThreadFactory,
    null,
    // we use asyncMode to ensure the queue is processed FIFO, which maximizes throughput
    true);

@Override
public void processOutput(Iterable<Target> partialResult)
    throws IOException, InterruptedException {
  ForkJoinTask<?> writeAllTargetsFuture;
  var targetQueue = new LinkedBlockingQueue<Future<List<byte[]>>>(MAX_CHUNKS_IN_QUEUE);
  var stillAddingTargetsToQueue = new AtomicBoolean(true);
  writeAllTargetsFuture = SHARED_EXECUTOR.submit(
      () -> {
Standards
  • ISO-IEC-25010-Performance-Resource-Utilization
  • Netflix-Resource-Pooling
  • Algorithm-Opt-Thread-Pool

Comment on lines +54 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource Leak Risk

The ForkJoinTask created inside the try-with-resources block continues execution after the ForkJoinPool is closed. This creates a resource leak risk as tasks may attempt to use the shutdown executor, potentially causing RejectedExecutionException or task failures.

        ForkJoinPool executor = new ForkJoinPool(
            Runtime.getRuntime().availableProcessors(),
            ForkJoinPool.defaultForkJoinWorkerThreadFactory,
            null,
            // we use asyncMode to ensure the queue is processed FIFO, which maximizes
            // throughput
            true);
        try {
Commitable Suggestion
Suggested change
try (ForkJoinPool executor =
new ForkJoinPool(
Runtime.getRuntime().availableProcessors(),
ForkJoinPool.defaultForkJoinWorkerThreadFactory,
null,
// we use asyncMode to ensure the queue is processed FIFO, which maximizes
// throughput
true)) {
ForkJoinPool executor = new ForkJoinPool(
Runtime.getRuntime().availableProcessors(),
ForkJoinPool.defaultForkJoinWorkerThreadFactory,
null,
// we use asyncMode to ensure the queue is processed FIFO, which maximizes
// throughput
true);
try {
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • SRE-Resource-Management

Comment on lines +54 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource Leak Risk

ForkJoinPool is created with no exception handler (null parameter). Uncaught exceptions in worker threads may cause thread termination without proper cleanup, potentially reducing parallelism effectiveness under load and causing resource leaks.

Standards
  • ISO-IEC-25010-Performance-Resource-Utilization
  • Concurrency-Pattern-Exception-Handling
  • Thread-Pool-Management

var targetQueue = new LinkedBlockingQueue<Future<List<byte[]>>>(MAX_CHUNKS_IN_QUEUE);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource Exhaustion Risk

The queue has a bounded size but no handling for queue full conditions. If producers outpace consumers, the put() operation will block indefinitely, potentially causing thread starvation or deadlock. Consider adding timeout mechanisms and graceful degradation when queue capacity is reached.

Standards
  • CWE-400
  • OWASP-A05
  • NIST-SSDF-PW.8

var stillAddingTargetsToQueue = new AtomicBoolean(true);
writeAllTargetsFuture =
executor.submit(
() -> {
try {
while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) {
Future<List<byte[]>> targets = targetQueue.take();
for (byte[] target : targets.get()) {
out.write(target);
}
Comment on lines +55 to +72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

ForkJoinPool worker is blocking on take()/get() – high risk of thread starvation

The consumer task runs inside the same ForkJoinPool that processes the producer tasks yet:

  • performs unmanaged blocking (targetQueue.take() and Future.get()),
  • uses a bounded queue that may fill up, causing the producer thread (caller of processOutput) to block on put().

Because ForkJoinPool counts blocked workers toward parallelism, the pool can dead-lock or under-utilise CPUs when many long-running producer tasks occupy the limited workers while the single consumer waits, or vice-versa.

Recommended approaches:

  1. Start the consumer on a dedicated thread (e.g. Executors.newSingleThreadExecutor) outside the FJP, or
  2. Wrap blocking calls with ForkJoinPool.managedBlock, or
  3. Replace the FJP entirely with a plain ExecutorService that tolerates blocking.

This will eliminate the starvation risk and make behaviour more predictable.

}
Comment on lines +68 to +73
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deadlock Potential

The consumer thread calls targetQueue.take() which blocks until an element is available. If an exception occurs during queue population and stillAddingTargetsToQueue remains true while the queue is empty, the consumer thread will deadlock waiting indefinitely for items that will never arrive.

                      while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) {
                        Future<List<byte[]>> targets;
                        try {
                          targets = targetQueue.poll(100, TimeUnit.MILLISECONDS);
                          if (targets == null) {
                            continue;
                          }
                        } catch (InterruptedException e) {
                          Thread.currentThread().interrupt();
                          throw new WrappedInterruptedException(e);
                        }
                        for (byte[] target : targets.get()) {
                          out.write(target);
                        }
Commitable Suggestion
Suggested change
while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) {
Future<List<byte[]>> targets = targetQueue.take();
for (byte[] target : targets.get()) {
out.write(target);
}
}
while (stillAddingTargetsToQueue.get() || !targetQueue.isEmpty()) {
Future<List<byte[]>> targets;
try {
targets = targetQueue.poll(100, TimeUnit.MILLISECONDS);
if (targets == null) {
continue;
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new WrappedInterruptedException(e);
}
for (byte[] target : targets.get()) {
out.write(target);
}
Standards
  • Algorithm-Correctness-Concurrency
  • Logic-Verification-Deadlock-Prevention

} catch (InterruptedException e) {
throw new WrappedInterruptedException(e);
} catch (IOException e) {
Comment on lines +74 to +76
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread Interruption Handling

InterruptedException is caught but the thread's interrupted status is not restored. When wrapping InterruptedException, the current thread's interrupt status should be preserved with Thread.currentThread().interrupt() to ensure proper interruption propagation.

                    } catch (InterruptedException e) {
                      Thread.currentThread().interrupt();
                      throw new WrappedInterruptedException(e);
Commitable Suggestion
Suggested change
} catch (InterruptedException e) {
throw new WrappedInterruptedException(e);
} catch (IOException e) {
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new WrappedInterruptedException(e);
Standards
  • ISO-IEC-25010-Reliability-Maturity
  • SRE-Error-Handling
  • ISO-IEC-25010-Functional-Correctness-Appropriateness

throw new WrappedIOException(e);
} catch (ExecutionException e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread Safety Issue: The ExecutionException handling is inadequate, using a generic RuntimeException with a TODO comment. This can mask important exceptions during task execution, potentially leading to resource leaks or thread pool exhaustion.

Consider unwrapping and properly handling the cause:

catch (ExecutionException e) {
  Throwable cause = e.getCause();
  if (cause instanceof IOException) {
    throw new WrappedIOException((IOException) cause);
  } else if (cause instanceof InterruptedException) {
    Thread.currentThread().interrupt(); // Preserve interrupt status
    throw new WrappedInterruptedException((InterruptedException) cause);
  } else if (cause instanceof RuntimeException) {
    throw (RuntimeException) cause;
  } else {
    throw new RuntimeException("Error processing targets", cause);
  }
}

// TODO: figure out what might be in here and propagate
throw new RuntimeException(e);
Comment on lines +78 to +80
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception Swallowing Risk

Generic exception handling wraps ExecutionException in RuntimeException without preserving cause details. Attackers could exploit swallowed exceptions to hide malicious activities. Proper exception handling needed for security visibility.

} catch (ExecutionException e) {
  Throwable cause = e.getCause();
  if (cause instanceof IOException) {
    throw new WrappedIOException((IOException) cause);
  } else if (cause instanceof InterruptedException) {
    throw new WrappedInterruptedException((InterruptedException) cause);
  } else {
    throw new RuntimeException("Unexpected error during target processing", e);
  }
Standards
  • CWE-390
  • CWE-755

}
Comment on lines +79 to +81

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This TODO indicates that the exception handling here is incomplete. It's important to propagate the original ExecutionException's cause to provide more context about the failure. Consider using e.getCause() to get the underlying exception and wrap it in a more specific exception type, or rethrow it if appropriate.

Also, consider logging the exception with sufficient context to aid debugging.

                      // TODO: figure out what might be in here and propagate
                      Throwable cause = e.getCause();
                      if (cause instanceof IOException) {
                        throw new WrappedIOException((IOException) cause);
                      } else if (cause instanceof InterruptedException) {
                        throw new WrappedInterruptedException((InterruptedException) cause);
                      } else {
                        throw new RuntimeException("Error during target processing", cause);
                      }

Comment on lines +78 to +81
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Error Propagation

ExecutionException caught with TODO comment and generic RuntimeException wrapping. Specific error information is lost, preventing proper error handling and recovery.

} catch (ExecutionException e) {
  Throwable cause = e.getCause();
  if (cause instanceof WrappedIOException) {
    throw ((WrappedIOException) cause).getCause();
  } else if (cause instanceof WrappedInterruptedException) {
    throw ((WrappedInterruptedException) cause).getCause();
  } else {
    throw new IOException("Failed to process target chunk", e);
  }
}
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness

Comment on lines +78 to +81
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncaught RuntimeException Propagation

Unresolved TODO with generic RuntimeException wrapping can mask actual failure causes. The code throws a generic RuntimeException without proper cause analysis, potentially hiding important error information and making debugging difficult in production.

                    } catch (ExecutionException e) {
                      Throwable cause = e.getCause();
                      if (cause instanceof IOException) {
                        throw new WrappedIOException((IOException) cause);
                      } else if (cause instanceof InterruptedException) {
                        throw new WrappedInterruptedException((InterruptedException) cause);
                      } else {
                        throw new RuntimeException("Error processing target data", e);
                      }
                    }
Commitable Suggestion
Suggested change
} catch (ExecutionException e) {
// TODO: figure out what might be in here and propagate
throw new RuntimeException(e);
}
} catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof IOException) {
throw new WrappedIOException((IOException) cause);
} else if (cause instanceof InterruptedException) {
throw new WrappedInterruptedException((InterruptedException) cause);
} else {
throw new RuntimeException("Error processing target data", e);
}
}
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • SRE-Error-Handling

});
try {
for (List<Target> targets : Iterables.partition(partialResult, TARGETS_PER_CHUNK)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concurrency Issue: The blocking targetQueue.put() operation could lead to thread starvation if the queue becomes full. There's no timeout or circuit-breaking mechanism to handle slow or stuck tasks.

Consider using a non-blocking approach with timeouts:

// Use offer with timeout to prevent indefinite blocking
Future<List<byte[]>> task = executor.submit(() -> writeTargetsDelimitedToByteArrays(targets));
if (!targetQueue.offer(task, 5, TimeUnit.SECONDS)) {
  // If queue is full for too long, process this batch directly to make progress
  for (byte[] target : task.get(30, TimeUnit.SECONDS)) {
    out.write(target);
  }
}

targetQueue.put(executor.submit(() -> writeTargetsDelimitedToByteArrays(targets)));
}
} finally {
stillAddingTargetsToQueue.set(false);
}
}
Comment on lines +88 to +90

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

After setting stillAddingTargetsToQueue to false, it's good practice to explicitly shutdown the executor and wait for its termination. This ensures that all submitted tasks are completed before proceeding. Otherwise, there is a risk that the executor will not shutdown cleanly, and resources may be leaked. Consider adding executor.shutdown() and executor.awaitTermination() calls within the finally block.

          } finally {
            stillAddingTargetsToQueue.set(false);
            executor.shutdown(); // Initiate shutdown
            try {
              if (!executor.awaitTermination(60, TimeUnit.SECONDS)) { // Wait for tasks to complete
                System.err.println("ForkJoinPool did not terminate in the specified timeout.");
                // Optionally, log the state of the tasks that are still running.
              }
            } catch (InterruptedException ie) {
              // (Re-)Cancel if current thread also interrupted
              executor.shutdownNow();
              // Preserve interrupt status
              Thread.currentThread().interrupt();
            }
          }

try {
writeAllTargetsFuture.get();
} catch (ExecutionException e) {
// TODO: propagate
throw new RuntimeException(e);
}
Comment on lines +94 to +96

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the previous TODO, this one also requires proper exception propagation. The ExecutionException should be unwrapped to get the root cause and either rethrown as a more specific exception or wrapped in a RuntimeException with the cause attached. Logging the exception is also recommended.

        } catch (ExecutionException e) {
          // TODO: propagate
          Throwable cause = e.getCause();
          if (cause instanceof IOException) {
            throw new WrappedIOException((IOException) cause);
          } else if (cause instanceof InterruptedException) {
            throw new WrappedInterruptedException((InterruptedException) cause);
          } else {
            throw new RuntimeException("Error during target processing", cause);
          }

Comment on lines +93 to +96
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception Handling Gap

The code catches ExecutionException but wraps it in a generic RuntimeException without preserving the original exception type. This loses important error context and makes debugging harder.

Suggested change
} catch (ExecutionException e) {
// TODO: propagate
throw new RuntimeException(e);
}
} catch (ExecutionException e) {
throw new WrappedExecutionException(e);
}
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • SRE-Error-Propagation

Comment on lines +92 to +96
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncaught Exceptions

ExecutionException is caught but wrapped in RuntimeException with a TODO comment. This loses specific error information and prevents proper error handling.

try {
  writeAllTargetsFuture.get();
} catch (ExecutionException e) {
  Throwable cause = e.getCause();
  if (cause instanceof WrappedIOException) {
    throw ((WrappedIOException) cause).getCause();
  } else if (cause instanceof WrappedInterruptedException) {
    throw ((WrappedInterruptedException) cause).getCause();
  } else {
    throw new IOException("Error processing targets", e);
  }
}
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness

Comment on lines +93 to +96
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unhandled Interrupted Exception

ExecutionException handling doesn't restore interrupted status when InterruptedException is the cause. Thread interruption signals could be lost, preventing proper thread termination. Security-sensitive operations might continue after intended shutdown.

} catch (ExecutionException e) {
  Throwable cause = e.getCause();
  if (cause instanceof InterruptedException) {
    Thread.currentThread().interrupt(); // Restore interrupted status
    throw new WrappedInterruptedException((InterruptedException) cause);
  } else if (cause instanceof IOException) {
    throw new WrappedIOException((IOException) cause);
  } else {
    throw new RuntimeException("Error processing targets", e);
  }
Standards
  • CWE-391
  • CWE-755

Comment on lines +93 to +96
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unhandled ExecutionException Propagation

Another unresolved TODO with generic RuntimeException wrapping. The code fails to properly analyze and handle the underlying cause of ExecutionException, which could contain important failure information needed for proper error handling and recovery.

        } catch (ExecutionException e) {
          Throwable cause = e.getCause();
          if (cause instanceof WrappedIOException) {
            throw ((WrappedIOException) cause).getCause();
          } else if (cause instanceof WrappedInterruptedException) {
            Thread.currentThread().interrupt();
            throw ((WrappedInterruptedException) cause).getCause();
          } else {
            throw new IOException("Error processing query results", e);
          }
        } finally {
          executor.shutdown();
        }
Commitable Suggestion
Suggested change
} catch (ExecutionException e) {
// TODO: propagate
throw new RuntimeException(e);
}
} catch (ExecutionException e) {
Throwable cause = e.getCause();
if (cause instanceof WrappedIOException) {
throw ((WrappedIOException) cause).getCause();
} else if (cause instanceof WrappedInterruptedException) {
Thread.currentThread().interrupt();
throw ((WrappedInterruptedException) cause).getCause();
} else {
throw new IOException("Error processing query results", e);
}
} finally {
executor.shutdown();
}
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • SRE-Error-Handling

Comment on lines +91 to +96
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unhandled Interrupted Exception

The code catches ExecutionException but doesn't handle InterruptedException from Future.get(). Thread interruption signals are lost, potentially causing thread leaks and preventing proper application shutdown. This can lead to resource exhaustion in long-running applications.

Standards
  • CWE-391
  • OWASP-A06
  • NIST-SSDF-PW.1

}

private List<byte[]> writeTargetsDelimitedToByteArrays(List<Target> targets) {
return targets.stream().map(target -> writeDelimited(toProto(target))).toList();
Comment on lines +99 to +100
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory Exhaustion Risk

The method processes all targets in memory without streaming or chunking large outputs. For large target lists, this could cause excessive memory consumption leading to OutOfMemoryError. Consider implementing true streaming with backpressure to prevent memory exhaustion.

Standards
  • CWE-400
  • OWASP-A05
  • NIST-SSDF-PW.8

}

private Build.Target toProto(Target target) {
try {
return toTargetProtoBuffer(target, ourLabelPrinter);
} catch (InterruptedException e) {
throw new WrappedInterruptedException(e);
}
}
};
}

private static byte[] writeDelimited(Build.Target targetProtoBuffer) {
try {
var serializedSize = targetProtoBuffer.getSerializedSize();
- var headerSize = CodedOutputStream.computeUInt32SizeNoTag(serializedSize);
- var output = new byte[headerSize + serializedSize];
- var codedOut = CodedOutputStream.newInstance(output, headerSize, output.length - headerSize);
- targetProtoBuffer.writeTo(codedOut);
+ int headerSize = CodedOutputStream.computeUInt32SizeNoTag(serializedSize);
+ byte[] output = new byte[headerSize + serializedSize];
+
+ // 1. write the var-int length prefix
+ CodedOutputStream headerOut = CodedOutputStream.newInstance(output, 0, headerSize);
+ headerOut.writeUInt32NoTag(serializedSize);
+ headerOut.flush();
+
+ // 2. write the message bytes immediately after the prefix
+ CodedOutputStream bodyOut =
+ CodedOutputStream.newInstance(output, headerSize, serializedSize);
+ targetProtoBuffer.writeTo(bodyOut);
+ bodyOut.flush();
Comment on lines +131 to +132
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synchronization Missing

Multiple threads write to the same output stream without synchronization. Concurrent writes to the output stream can cause data corruption as bytes from different threads interleave unpredictably.

// Add synchronization when writing to the output stream
synchronized(out) {
    targetProtoBuffer.writeTo(bodyOut);
    bodyOut.flush();
}
Standards
  • Algorithm-Correctness-Concurrency
  • Logic-Verification-Thread-Safety
  • Business-Rule-Data-Integrity

Comment on lines +124 to +132
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclosed Resource Leak

CodedOutputStream instances are not properly closed. The streams implement Closeable interface and should be closed to prevent resource leaks, especially in high-throughput concurrent processing.

// Use try-with-resources to ensure streams are closed properly
try (CodedOutputStream headerOut = CodedOutputStream.newInstance(output, 0, headerSize)) {
    headerOut.writeUInt32NoTag(serializedSize);
    headerOut.flush();
}

// 2. write the message bytes immediately after the prefix
try (CodedOutputStream bodyOut = CodedOutputStream.newInstance(output, headerSize, serializedSize)) {
    targetProtoBuffer.writeTo(bodyOut);
    bodyOut.flush();
}
Standards
  • Logic-Verification-Resource-Management
  • Algorithm-Correctness-Resource-Handling
  • Business-Rule-Resource-Cleanup

Comment on lines +124 to +132
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant CodedOutputStream Creation

Creating separate CodedOutputStream instances for header and body adds overhead. A single CodedOutputStream with appropriate positioning would be more efficient, reducing object creation and improving serialization performance under high throughput.

Standards
  • ISO-IEC-25010-Performance-Resource-Utilization
  • Optimization-Pattern-Object-Reuse
  • Memory-Allocation-Efficiency

return output;
}
} catch (IOException e) {
Comment on lines +134 to +135
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Syntax Error

Syntax error in the writeDelimited method. The code has an extra closing bracket after 'return output;' but before the catch block, which will prevent compilation and cause build failures. This breaks the try-catch structure.

      return output;
Commitable Suggestion
Suggested change
}
} catch (IOException e) {
return output;
Standards
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • ISO-IEC-25010-Reliability-Maturity

throw new WrappedIOException(e);
}
Comment on lines +134 to +137
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclosed Resources Risk

The method has an extra closing brace that creates a syntax error. This will cause compilation failure and prevent the code from functioning properly.

Suggested change
}
} catch (IOException e) {
throw new WrappedIOException(e);
}
} catch (IOException e) {
throw new WrappedIOException(e);
}
}
Standards
  • ISO-IEC-25010-Reliability-Maturity
  • ISO-IEC-25010-Functional-Correctness

Comment on lines +134 to +137
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Syntax Error

Syntax error with mismatched curly braces. The extra closing brace on line 135 breaks the method structure, causing compilation failure.

      return output;
    } catch (IOException e) {
      throw new WrappedIOException(e);
    }
Standards
  • ISO-IEC-25010-Functional-Correctness-Appropriateness

Comment on lines +134 to +137
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken Exception Chain

The writeDelimited method has mismatched braces causing a logical syntax error. The extra closing brace at line 135 will prevent compilation, and the exception handling block at lines 136-137 is improperly structured, breaking the intended error propagation flow.

Standards
  • Algorithm-Correctness-Syntax
  • Logic-Verification-Error-Handling

}

private static class WrappedIOException extends RuntimeException {
private WrappedIOException(IOException cause) {
super(cause);
}

@Override
public IOException getCause() {
return (IOException) super.getCause();
Comment on lines +145 to +147

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider whether wrapping the IOException in a RuntimeException is truly necessary. If the calling code can handle IOException directly, it might be better to avoid the wrapping. If wrapping is necessary, ensure that the original exception is accessible via getCause().

    @Override
    public IOException getCause() {
      return cause;
    }

}
}

private static class WrappedInterruptedException extends RuntimeException {
private WrappedInterruptedException(InterruptedException cause) {
super(cause);
}

@Override
public InterruptedException getCause() {
return (InterruptedException) super.getCause();
}
Comment on lines +157 to +159

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to WrappedIOException, evaluate the necessity of wrapping InterruptedException. If the calling code can handle it directly, avoid wrapping. If wrapping is necessary, ensure the original exception is accessible via getCause().

    @Override
    public InterruptedException getCause() {
      return cause;
    }

}
Comment on lines +140 to +160
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapper Classes Duplication

Two nearly identical wrapper exception classes follow the same pattern with duplicated code structure. This violates DRY principle and increases maintenance burden. A generic wrapper with type parameter or unified exception handling strategy would be more maintainable.

Standards
  • Clean-Code-DRY
  • SOLID-DRY
  • Maintainability-Quality-Duplication

}