feat: implement preserve_fds support and fix file descriptor handling

[ComixHe]

• Add --preserve-fds command line option for runtime
• Refactor container creation with unified options structure
• Improve file_descriptor class API

[deepsource-io]

Here's the code health analysis summary for commits b1c7fa1..c9c71a6. View details on DeepSource ↗.

Analysis Summary

Analyzer	Status	Summary	Link
C & C++	❌ Failure	❗ 5 occurences introduced 🎯 1 occurence resolved	View Check ↗
Shell	✅ Success		View Check ↗
Secrets	✅ Success		View Check ↗
Test coverage	⚠️ Artifact not reported	Timed out: Artifact was never reported	View Check ↗

---

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

[codereviewbot-ai]

Security Concern: Default Bundle Path

The default value for the bundle path is set to the current directory (.). This could pose a security risk if the directory contains sensitive files or is not the intended directory for OCI bundles. It is recommended to either:

• Set a more secure and explicit default path that is less likely to contain sensitive or unrelated files.
• Implement validation to ensure the provided path is safe and correct, possibly rejecting paths that do not meet specific criteria.

[codereviewbot-ai]

Maintainability Issue: Hardcoded Default Values

The default values for the bundle path and configuration file are hardcoded ('.' and 'config.json' respectively). This approach lacks flexibility and might not suit all deployment environments or use cases. Consider allowing these defaults to be configurable through environment variables or a configuration file, enhancing the adaptability and maintainability of the application.

[codereviewbot-ai]

The members ID, bundle, config, and preserve_fds in the run_options struct are not initialized in the constructor, which can lead to undefined behavior if they are accessed before being set. Recommendation: Initialize all members within the constructor to ensure they have valid values.

Example:

run_options(global_options &global) : global_(global), ID(""), bundle(""), config(""), preserve_fds(0) {}

[codereviewbot-ai]

The preserve_fds member uses uint, which is not a standard type in C++. This can lead to portability issues across different compilers and platforms. Recommendation: Use unsigned int or another standard C++ type to ensure compatibility.

Example:

unsigned int preserve_fds;

[codereviewbot-ai]

Error Handling and Exception Safety

The code lacks explicit error handling for operations that could potentially fail, such as the construction of runtime_t and create_container. Failure in these operations could lead to unhandled exceptions or undefined behaviors.

Recommendation:

• Implement try-catch blocks around the operations that could throw exceptions.
• Check the validity of the container object before calling run on it to prevent potential crashes if create_container fails.

[codereviewbot-ai]

Returning container by value in create_container function might cause performance inefficiencies if container is a large or complex object. If the design allows, returning a smart pointer or a reference could be more efficient, especially if object ownership needs to be shared or transferred.

Suggested Change:

auto create_container(const create_container_options_t &options) -> std::unique_ptr<container>;

[codereviewbot-ai]

Using the move assignment operator within the move constructor is unconventional. Typically, the move constructor should directly initialize its members rather than delegating to the move assignment operator. This can improve performance by eliminating unnecessary operations:

file_descriptor(file_descriptor&& other) noexcept : fd_(other.fd_) {
    other.fd_ = -1;
}

This change directly sets fd_ and marks the other's file descriptor as invalid, which is more straightforward and efficient.

[codereviewbot-ai]

The release method attempts to close a file descriptor without checking if it was valid (non-negative). This could lead to unnecessary system call errors if the descriptor is invalid. It's recommended to add a check before attempting to close the descriptor:

if (ret >= 0 && ::close(ret) < 0) {
    auto msg = "failed to close file descriptor " + std::to_string(ret) + ": " + ::strerror(errno);
    throw file_descriptor_invalid_exception(msg);
}

This ensures that close is only called with valid descriptors, preventing misleading errors.

[codereviewbot-ai]

The implementation details of the move assignment operator (operator=(file_descriptor &&other) noexcept) are not visible here. It's crucial to ensure that this operator handles self-assignment gracefully and manages the resources (file descriptors) correctly to prevent resource leaks or double deletion issues. Recommended Solution: Implement the move assignment operator to first check for self-assignment, release any existing resources, and then transfer ownership of the resources from the source object.

[codereviewbot-ai]

The duplicate method is expected to handle system calls that may fail, such as dup or dup2. It is important to ensure that this method handles such failures by throwing appropriate exceptions or returning an error code. This is crucial for robust error handling and to prevent the application from operating on an invalid file descriptor. Recommended Solution: Implement error handling in the duplicate method to check the return value of the system call and throw an exception or handle the error if the duplication fails.