feat: add runtime extension 'cn.org.linyaps.runtime.ns_last_pid'#103
Conversation
Add support for the 'cn.org.linyaps.runtime.ns_last_pid' runtime extension that allows setting the last PID in the namespace during container startup. 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}
| } | ||
|
|
||
| if (h.contains("env")) { | ||
| std::map<std::string, std::string> env; | ||
| std::unordered_map<std::string, std::string> env; | ||
|
|
||
| for (const auto &e : h["env"].get<std::vector<std::string>>()) { | ||
| auto pos = e.find('='); |
There was a problem hiding this comment.
The error handling for environment variables checks for the presence of '=' to split the string into key-value pairs. However, the error message "invalid env entry" could be more descriptive. It would be beneficial to include the actual content of the erroneous entry in the error message to aid in debugging.
Suggested Improvement:
throw std::runtime_error("invalid env entry, expected format 'key=value': " + e);| cfg.root.readonly = j[root / "readonly"].get<bool>(); | ||
| } | ||
|
|
||
| auto annotations = ptr / "annotations"; | ||
| if (j.contains(annotations)) { | ||
| cfg.annotations = j[annotations].get<std::unordered_map<std::string, std::string>>(); | ||
| } | ||
|
|
||
| return cfg; | ||
| } | ||
|
|
There was a problem hiding this comment.
The parsing of the 'readonly' property from the JSON object is straightforward and correctly retrieves the boolean value. However, there is a potential improvement in the handling of the 'annotations' parsing. The code directly retrieves and assigns the annotations without checking the integrity or validity of the data.
Suggested Improvement:
Consider validating the keys and values of the annotations to ensure they meet expected formats or constraints, especially if they are used in critical parts of the application. This can prevent issues related to unexpected data causing errors downstream.
| #include <sys/mount.h> | ||
|
|
||
| #include <filesystem> | ||
| #include <map> | ||
| #include <optional> | ||
| #include <unordered_map> | ||
| #include <vector> | ||
|
|
||
| #include <sys/resource.h> |
There was a problem hiding this comment.
The inclusion of system-specific headers such as <sys/mount.h> and <sys/resource.h> directly in a widely included header file can lead to portability issues and increased compile-time dependencies. Consider using forward declarations or moving these includes to implementation files if possible to reduce coupling and improve compilation times.
| { | ||
| std::filesystem::path path; | ||
| std::optional<std::vector<std::string>> args; | ||
| std::optional<std::map<std::string, std::string>> env; | ||
| std::optional<std::unordered_map<std::string, std::string>> env; | ||
| std::optional<int> timeout; | ||
| }; | ||
|
|
There was a problem hiding this comment.
The hook_t struct lacks validation for the path field to ensure it points to a valid executable. This could lead to security risks if the path is misconfigured. Implementing a validation mechanism or ensuring that paths are checked at runtime before use would enhance security and robustness.
| }; | ||
|
|
||
| root_t root; | ||
|
|
||
| std::optional<std::unordered_map<std::string, std::string>> annotations; | ||
| }; | ||
|
|
||
| } // namespace linyaps_box |
There was a problem hiding this comment.
The root_t struct's readonly field defaults to false, which might not be the safest default for a configuration that might require high security or immutability. Consider setting the default value of readonly to true to enforce stricter security by default, and allow explicit configuration to make it writable if necessary.
- non-const global variable - avoid array-to-pointer decay Signed-off-by: ComixHe <ComixHe1895@outlook.com>
| if (::fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { | ||
| throw std::system_error(errno, | ||
| std::generic_category(), | ||
| std::string{ "failed to set up close-on-exec to " } | ||
| + next->d_name); | ||
| "failed to set up close-on-exec to " + name); | ||
| } | ||
| } else { | ||
| if (::close(fd) < 0) { | ||
| throw std::system_error(errno, | ||
| std::generic_category(), | ||
| std::string{ "failed to close " } + next->d_name); | ||
| throw std::system_error(errno, std::generic_category(), "failed to close " + name); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The error messages generated when fcntl or close fail use the variable name, which misleadingly contains only the first character of the file descriptor. This can lead to confusion when diagnosing issues from logs.
Recommendation:
Modify the error messages to use the full file descriptor number instead of the truncated name. This can be achieved by converting the file descriptor number to a string and using it in the error message.
| auto fd = std::stoi(name.c_str()); | ||
| if (fd == self_fd) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The conversion from std::string to int using std::stoi (line 62) and subsequent comparison with unsigned integers could lead to issues if std::stoi returns a negative value, which isn't currently handled.
Recommendation:
Add a check to ensure that the file descriptor (fd) is non-negative before proceeding with further logic. This will prevent potential undefined behavior or errors when casting negative integers to unsigned types.
| bool force_log_to_stderr() | ||
| { | ||
| static auto *result = getenv("LINYAPS_BOX_LOG_FORCE_STDERR"); | ||
| static const auto *result = getenv("LINYAPS_BOX_LOG_FORCE_STDERR"); | ||
| return result != nullptr; |
There was a problem hiding this comment.
The use of getenv in a potentially multi-threaded context (such as logging functions) can lead to race conditions if the environment is modified concurrently. This can cause undefined behavior or crashes.
Recommendation:
Consider using thread-safe alternatives to manage environment variables, or ensure that environment variables are not modified after program initialization to prevent such issues.
1 participant
Add support for the 'cn.org.linyaps.runtime.ns_last_pid' runtime extension that allows setting the last PID in the namespace during container startup.