UTL: unique file name per process #9143
Conversation
Signed-off-by: osamahammad21 <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to make temporary filenames unique per process by embedding the process ID, which is a good step to prevent collisions in concurrent environments. However, the change introduces a critical issue by breaking existing unit tests that rely on the old filename format. Additionally, a pre-existing high-severity security vulnerability (TOCTOU) remains in the filename generation logic. I have provided detailed comments on both of these points.
| { | ||
| int counter = 1; | ||
| std::string filename; | ||
| const auto pid = static_cast<unsigned int>(::getpid()); | ||
| do { | ||
| filename = fmt::format("{}.{}", prefix, counter++); | ||
| filename = fmt::format("{}.{}.{}", prefix, pid, counter++); | ||
| } while (std::filesystem::exists(filename)); | ||
|
|
||
| return filename; |
There was a problem hiding this comment.
This change to the filename format will break existing tests. The tests stream_handler_temp_file_handling and file_handler_temp_file_handling in src/utl/test/cpp/TestCFileUtils.cpp construct an expected temporary filename (e.g., test_temp_file_handling.txt.1) which will no longer match the new format (test_temp_file_handling.txt.<pid>.1). The tests need to be updated to work with the new filename generation logic.
| { | ||
| int counter = 1; | ||
| std::string filename; | ||
| const auto pid = static_cast<unsigned int>(::getpid()); | ||
| do { | ||
| filename = fmt::format("{}.{}", prefix, counter++); | ||
| filename = fmt::format("{}.{}.{}", prefix, pid, counter++); | ||
| } while (std::filesystem::exists(filename)); | ||
|
|
||
| return filename; |
There was a problem hiding this comment.
While adding the process ID is a good improvement to prevent filename collisions between concurrent processes, this function is still vulnerable to a Time-of-check to time-of-use (TOCTOU) race condition. There is a time window between the check for file existence (std::filesystem::exists) and the file's actual creation by the caller. An attacker could exploit this by creating a symbolic link with the predicted filename, potentially causing the application to write to an unintended file. A more secure approach is to use mkstemp, which atomically creates a unique file, as is already done in the ScopedTemporaryFile class.
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: osamahammad21 <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| { | ||
| const char* filename = "test_temp_file_handling.txt"; | ||
| std::string tmp_filename = std::string(filename) + ".1"; | ||
| const auto pid = static_cast<unsigned int>(::getpid()); |
There was a problem hiding this comment.
warning: no header providing "getpid" is directly included [misc-include-cleaner]
src/utl/test/cpp/TestCFileUtils.cpp:3:
- #include <cstdint>
+ #include <unistd.h>
+ #include <cstdint>Signed-off-by: osamahammad21 <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 participant
No description provided.