fix: handle broken symlinks during mount destination creation#113
Conversation
When a destination file doesn't exist, the previous implementation couldn't distinguish between creation failure due to existing files vs other reasons. If both opening and creating fail, the destination is definitely a broken symlink. - Add recursive symlink resolution with depth limit (32) in create_destination_file() - Use O_NOFOLLOW to detect symlink during file creation - When creation fails with ELOOP, read symlink target and recursively create it - Replace filesystem functions with internal utils for consistent error handling - Add proper broken symlink detection and resolution logic This ensures mount destinations work correctly even when they point to broken symlinks, by creating the missing target files in the symlink chain. Signed-off-by: ComixHe <ComixHe1895@outlook.com>
|
Here's the code health analysis summary for commits Analysis Summary
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| auto ret = dup(fd_); | ||
| if (ret < 0) { | ||
| throw std::system_error(errno, std::generic_category(), "fcntl"); |
There was a problem hiding this comment.
The method uses dup to duplicate the file descriptor, which does not allow setting flags such as FD_CLOEXEC that are crucial for security, especially to prevent file descriptor leaks across fork-exec calls.
Recommendation: Use dup3 instead of dup, which allows setting flags directly during the duplication process. This change would enhance security by ensuring that file descriptors are not unintentionally inherited by child processes.
| @@ -30,3 +34,26 @@ void linyaps_box::utils::symlink_at(const std::filesystem::path &target, | |||
There was a problem hiding this comment.
Potential Overwriting of Existing Symlinks
The function linyaps_box::utils::symlink does not check if the link_path already exists before attempting to create a new symlink. This could lead to unintentional overwriting of existing files or links, which might be critical.
Recommendation:
Add a check to see if link_path exists before creating the symlink. If it does, handle the situation appropriately, possibly by informing the user or logging a warning.
There was a problem hiding this comment.
Potential Buffer Overflow and Incomplete Path Handling in readlinkat
The function linyaps_box::utils::readlinkat uses a fixed-size buffer based on PATH_MAX to read the symlink target. This approach can lead to buffer overflow if the actual path exceeds PATH_MAX characters. Additionally, there is no verification that the entire path was successfully read into the buffer.
Recommendation:
Consider dynamically allocating the buffer based on the actual size needed for the path. Also, ensure that the path read is complete and handle cases where the path might exceed PATH_MAX safely.
| const file_descriptor &dirfd, | ||
| const std::filesystem::path &link_path); |
There was a problem hiding this comment.
The function symlink_at uses a file_descriptor which should be validated to ensure it refers to a valid directory before attempting to create a symlink. This is crucial to prevent operations on invalid or unintended directories, which could lead to security issues or system instability.
Recommended Solution:
Implement a check at the beginning of the function to verify that dirfd is a valid directory descriptor. If the check fails, the function should return an error or throw an exception to handle this gracefully.
| std::filesystem::path readlinkat(const file_descriptor &dirfd, const std::filesystem::path &path); | ||
|
|
There was a problem hiding this comment.
The function readlinkat also uses a file_descriptor and should ensure that this descriptor is valid and points to an actual directory. Similar to symlink_at, improper handling of dirfd can lead to reading links from incorrect or unintended directories.
Recommended Solution:
Include a validation step for dirfd to confirm it's a valid directory descriptor. If invalid, the function should handle the error appropriately, either by returning a specific error code or throwing an exception.
| const auto fd = ::openat(root.get(), path.c_str(), O_CREAT | O_WRONLY, 0666); | ||
| const auto fd = ::openat(root.get(), path.c_str(), flag, mode); | ||
| if (fd == -1) { | ||
| throw std::system_error(errno, |
There was a problem hiding this comment.
The function directly uses the flag and mode parameters in the openat system call without any checks or sanitization. This could potentially lead to security vulnerabilities if inappropriate flags or modes are passed, such as creating files with overly permissive access rights. It is recommended to add checks or sanitizations for these parameters to ensure they meet the security requirements of your application. For instance, ensuring that the mode does not inadvertently set world-writable permissions unless explicitly intended.
| auto touch(const file_descriptor &root, | ||
| const std::filesystem::path &path, | ||
| int flag, | ||
| mode_t mode = 0700) -> file_descriptor; |
There was a problem hiding this comment.
The function touch lacks explicit error handling mechanisms in its signature. This could lead to issues when file operations fail, as the method of error communication to the caller is not defined. Recommendation: Consider modifying the function to return a result or status code, or throw exceptions to handle different error conditions effectively.
| auto touch(const file_descriptor &root, | ||
| const std::filesystem::path &path, | ||
| int flag, | ||
| mode_t mode = 0700) -> file_descriptor; |
There was a problem hiding this comment.
The default file mode 0700 in the touch function is secure but may not be suitable for all scenarios, especially in a multi-user environment where different permissions might be necessary. Recommendation: Provide an interface to specify different permissions or document the rationale behind the chosen default to ensure it meets the intended use cases.
1 participant
When a destination file doesn't exist, the previous implementation couldn't distinguish between creation failure due to existing files vs other reasons. If both opening and creating fail, the destination is definitely a broken symlink.
This ensures mount destinations work correctly even when they point to broken symlinks, by creating the missing target files in the symlink chain.