Add --file_write_strategy to Bazel
#9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java # src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java # src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java # Conflicts: # src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
� Conflicts: � src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java � src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java � src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java
# Conflicts: # src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java # src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Secure Implementation of Lazy File Writing Strategy👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| public InputStream getInputStream() { | ||
| // TODO: Avoid materializing the full content in memory by using a variant of | ||
| // Piped{Input,Output}Stream that works well with virtual threads. | ||
| var out = new ByteArrayOutputStream(Math.clamp(getSize(), 0, Integer.MAX_VALUE)); |
There was a problem hiding this comment.
Potential Memory Exhaustion in Inline File Handling
The code attempts to allocate a ByteArrayOutputStream with a size potentially as large as Integer.MAX_VALUE. While there is a clamp function to limit the size, this could still lead to memory exhaustion if a malicious or corrupted file has a very large reported size. This could be exploited as a denial of service attack by causing the JVM to run out of memory when processing large files.
Suggested change
| var out = new ByteArrayOutputStream(Math.clamp(getSize(), 0, Integer.MAX_VALUE)); | |
| // Define a reasonable maximum size (e.g., 10MB) | |
| private static final int MAX_IN_MEMORY_SIZE = 10 * 1024 * 1024; | |
| var out = new ByteArrayOutputStream(Math.clamp(getSize(), 0, Math.min(MAX_IN_MEMORY_SIZE, Integer.MAX_VALUE))); |
Standards
- CWE-400
- A05:2021-Security Misconfiguration
Comment on lines
+1079
to
+1090
| public InputStream getInputStream() { | ||
| // TODO: Avoid materializing the full content in memory by using a variant of | ||
| // Piped{Input,Output}Stream that works well with virtual threads. | ||
| var out = new ByteArrayOutputStream(Math.clamp(getSize(), 0, Integer.MAX_VALUE)); | ||
| try { | ||
| writeTo(out); | ||
| } catch (IOException e) { | ||
| // writer is not expected to throw if out doesn't. | ||
| throw new IllegalStateException(e); | ||
| } | ||
| return new ByteArrayInputStream(out.toByteArray()); | ||
| } |
There was a problem hiding this comment.
Potential Resource Leak in getInputStream Method
The getInputStream method creates a ByteArrayOutputStream, writes to it, and then returns a ByteArrayInputStream without closing the ByteArrayOutputStream. While ByteArrayOutputStream doesn't use system resources that need explicit closing, this pattern could lead to resource leaks with other stream implementations or if the implementation changes in the future. Additionally, the exception handling assumes the writer won't throw if the output stream doesn't, which may not always be true.
Suggested change
| public InputStream getInputStream() { | |
| // TODO: Avoid materializing the full content in memory by using a variant of | |
| // Piped{Input,Output}Stream that works well with virtual threads. | |
| var out = new ByteArrayOutputStream(Math.clamp(getSize(), 0, Integer.MAX_VALUE)); | |
| try { | |
| writeTo(out); | |
| } catch (IOException e) { | |
| // writer is not expected to throw if out doesn't. | |
| throw new IllegalStateException(e); | |
| } | |
| return new ByteArrayInputStream(out.toByteArray()); | |
| } | |
| public InputStream getInputStream() { | |
| // TODO: Avoid materializing the full content in memory by using a variant of | |
| // Piped{Input,Output}Stream that works well with virtual threads. | |
| try (var out = new ByteArrayOutputStream(Math.clamp(getSize(), 0, Integer.MAX_VALUE))) { | |
| try { | |
| writeTo(out); | |
| return new ByteArrayInputStream(out.toByteArray()); | |
| } catch (IOException e) { | |
| // writer is not expected to throw if out doesn't. | |
| throw new IllegalStateException(e); | |
| } | |
| } | |
| } |
Standards
- CWE-772
- A06:2021-Vulnerable and Outdated Components
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Work towards bazelbuild#24808
RELNOTES: If the new
--file_write_strategyflag is set tolazy, files produced by file write actions (e.g. the ones produced byctx.actions.writeandctx.actions.expand_template) are not written to disk with BwoB unless required by local execution or upon explicit request. The default iseager, which retains the current behavior.