fix(python): avoid FileAlreadyExistsException in venv cleanup#373
Open
fix(python): avoid FileAlreadyExistsException in venv cleanup#373
Conversation
Remove redundant Files.createFile() before Files.write() in PythonControllerVirtualEnv. Files.write() already creates the file if it doesn't exist. The createFile() call caused FileAlreadyExistsException when a previous analysis failed mid-way and left the requirements.txt behind in /tmp/trustify_da_env/. Implements TC-3894 Assisted-by: Claude Code
Contributor
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoRemove redundant file creation in Python venv cleanup
WalkthroughsDescription• Remove redundant Files.createFile() call before Files.write() • Files.write() already creates file if missing • Fixes FileAlreadyExistsException when retrying failed analyses • Prevents stale requirements.txt from causing race conditions Diagramflowchart LR
A["Previous: createFile + write"] -->|"FileAlreadyExistsException"| B["Failed on retry"]
C["Fixed: write only"] -->|"Auto-creates file"| D["Works on retry"]
File Changes1. src/main/java/io/github/guacsec/trustifyda/utils/PythonControllerVirtualEnv.java
|
Contributor
Code Review by Qodo
1. TOCTOU write in /tmp
|
Contributor
Test Results379 tests 379 ✅ 1m 46s ⏱️ Results for commit 420c7e3. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
91
to
96
| Files.deleteIfExists(envRequirements); | ||
| String freezeOutput = | ||
| Operations.runProcessGetOutput(pythonEnvironmentDir, pipBinaryLocation, "freeze"); | ||
| Files.createFile(envRequirements); | ||
| Files.write(envRequirements, freezeOutput.getBytes()); | ||
| Operations.runProcessGetOutput( | ||
| pythonEnvironmentDir, pipBinaryLocation, "uninstall", "-y", "-r", "requirements.txt"); |
Contributor
There was a problem hiding this comment.
1. Toctou write in /tmp 🐞 Bug ⛨ Security
PythonControllerVirtualEnv.cleanEnvironment() deletes requirements.txt, runs a non-trivial external command (pip freeze), then writes requirements.txt; after removing Files.createFile(), a file recreated during that window will now be overwritten instead of failing fast. Because the venv directory is a fixed /tmp path and prepareEnvironment() trusts any pre-existing path, this enables symlink/race-style clobbering and can corrupt cleanup behavior.
Agent Prompt
### Issue description
`cleanEnvironment(false)` deletes `/tmp/trustify_da_env/requirements.txt`, runs `pip freeze`, then writes `requirements.txt`. With the removal of `Files.createFile()`, a path recreated between delete and write will be overwritten, creating a TOCTOU window on a `/tmp`-based directory.
### Issue Context
- The environment directory is hard-coded under `/tmp` and `prepareEnvironment()` trusts any existing path at that location.
- `pip freeze` introduces a time gap between delete and write.
### Fix Focus Areas
- src/main/java/io/github/guacsec/trustifyda/utils/PythonControllerVirtualEnv.java[39-43]
- src/main/java/io/github/guacsec/trustifyda/utils/PythonControllerVirtualEnv.java[89-96]
### Proposed fix
1. Write freeze output to a temp file in `pythonEnvironmentDir` (e.g., `Files.createTempFile(pythonEnvironmentDir, "requirements", ".txt")`).
2. Atomically replace the final `requirements.txt` using `Files.move(temp, envRequirements, ATOMIC_MOVE, REPLACE_EXISTING)` (fallback to non-atomic move if ATOMIC_MOVE unsupported).
3. (Optional hardening) Validate `pythonEnvironmentDir` is a real directory (not a symlink) before using it, and consider creating a per-run temp venv directory instead of a fixed `/tmp/trustify_da_env` path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Files.createFile()beforeFiles.write()inPythonControllerVirtualEnvFiles.write()already creates the file if it doesn't existcreateFile()call causedFileAlreadyExistsExceptionwhen a previous analysis failed mid-way and left therequirements.txtbehind in/tmp/trustify_da_env/Test plan
FileAlreadyExistsExceptionImplements TC-3894
🤖 Generated with Claude Code