gemini-code-assist[bot] commented on code in PR #19840:
URL: https://github.com/apache/tvm/pull/19840#discussion_r3441702364
##########
apps/cpp_rpc/rpc_env.cc:
##########
@@ -298,8 +248,13 @@ std::string BuildSharedLibrary(std::string file) {
if (support::EndsWith(file, ".o")) {
CreateShared(file_name, {file});
} else if (support::EndsWith(file, ".tar")) {
- const std::string tmp_dir = "./rpc/tmp/";
- mkdir(tmp_dir.c_str(), 0777);
+ const std::string tmp_dir = (std::filesystem::path(base_) /
"tmp").string();
+ std::filesystem::create_directories(tmp_dir);
+ std::error_code perm_ec;
+ std::filesystem::permissions(tmp_dir,
+ std::filesystem::perms::all,
+ std::filesystem::perm_options::replace,
+ perm_ec);
Review Comment:
Similar to the `base_` directory, setting `tmp_dir` permissions to
`std::filesystem::perms::all` (0777) allows any local user to modify files
inside the temporary directory before they are compiled/linked into a shared
library. This can lead to local privilege escalation or arbitrary code
execution.
Please restrict the permissions to `std::filesystem::perms::owner_all`
(0700) to ensure only the owner of the process can read/write to this directory.
```suggestion
std::error_code perm_ec;
std::filesystem::permissions(tmp_dir,
std::filesystem::perms::owner_all,
std::filesystem::perm_options::replace,
perm_ec);
```
##########
apps/cpp_rpc/rpc_server.cc:
##########
@@ -176,41 +178,44 @@ class RPCServer {
#if defined(__linux__) || defined(__ANDROID__) || defined(__APPLE__)
// step 3: serving
if (timeout != 0) {
- const pid_t timer_pid = fork();
- if (timer_pid == 0) {
- // Timer process
- sleep(timeout);
- _exit(0);
- }
-
const pid_t worker_pid = fork();
if (worker_pid == 0) {
// Worker process
ServerLoopProc(conn, addr, work_dir_);
_exit(0);
}
- int status_first = 0;
- const pid_t finished_first = waitPidEintr(&status_first);
- if (finished_first == timer_pid) {
- kill(worker_pid, SIGTERM);
- } else if (finished_first == worker_pid) {
- kill(timer_pid, SIGTERM);
- } else {
- LOG(INFO) << "Child pid=" << finished_first << " unexpected, but
still continue.";
- }
+ std::mutex mtx;
+ std::condition_variable cv;
+ bool timed_out = false;
+ bool worker_finished = false;
- int status_second = 0;
- waitPidEintr(&status_second);
+ // Timer thread
+ std::thread watchdog([&]() {
+ std::unique_lock<std::mutex> lock(mtx);
+ if (!cv.wait_for(lock, std::chrono::seconds(timeout), [&] { return
worker_finished; })) {
+ timed_out = true;
+ kill(worker_pid, SIGTERM);
+ }
+ });
Review Comment:
There is a potential race condition (CWE-362) between the parent thread
reaping the worker process and the watchdog thread timing out:
1. The worker process exits.
2. The parent thread's `waitPidEintr(worker_pid, &status)` reaps the worker
process. The PID `worker_pid` is now released and can be recycled by the OS.
3. Before the parent thread can acquire `mtx` and set `worker_finished =
true`, the parent thread is preempted.
4. A new process is spawned by the OS and gets assigned the recycled
`worker_pid`.
5. The watchdog thread times out, acquires `mtx`, sees `worker_finished` is
still `false`, and calls `kill(worker_pid, SIGTERM)`.
6. This sends `SIGTERM` to a completely unrelated process.
To mitigate this, consider checking if the process has exited without
reaping it first (e.g., using `waitpid` with `WNOWAIT` on supported platforms),
or ensure that the watchdog thread is safely joined/stopped before the parent
thread reaps the child process.
##########
apps/cpp_rpc/rpc_env.cc:
##########
@@ -298,8 +248,13 @@ std::string BuildSharedLibrary(std::string file) {
if (support::EndsWith(file, ".o")) {
CreateShared(file_name, {file});
} else if (support::EndsWith(file, ".tar")) {
- const std::string tmp_dir = "./rpc/tmp/";
- mkdir(tmp_dir.c_str(), 0777);
+ const std::string tmp_dir = (std::filesystem::path(base_) /
"tmp").string();
+ std::filesystem::create_directories(tmp_dir);
Review Comment:
Use the `std::error_code` overload for `std::filesystem::create_directories`
to avoid throwing unhandled exceptions.
```suggestion
std::error_code ec;
std::filesystem::create_directories(tmp_dir, ec);
```
##########
apps/cpp_rpc/rpc_env.cc:
##########
@@ -69,38 +57,41 @@ namespace tvm {
namespace runtime {
RPCEnv::RPCEnv(const std::string& wd) {
- if (wd != "") {
+ if (!wd.empty()) {
base_ = wd + "/.cache";
- mkdir(wd.c_str(), 0777);
- mkdir(base_.c_str(), 0777);
+ std::filesystem::create_directories(base_);
} else {
#if defined(ANDROID) || defined(__ANDROID__)
- char cwd[PATH_MAX];
- auto cmdline = fopen("/proc/self/cmdline", "r");
- fread(cwd, 1, sizeof(cwd), cmdline);
- fclose(cmdline);
- std::string android_base_ = "/data/data/" + std::string(cwd) + "/cache";
- struct stat statbuf;
+ std::string pkg_name;
+ if (std::ifstream cmdline("/proc/self/cmdline"); cmdline) {
+ std::getline(cmdline, pkg_name, '\0');
+ }
+ std::string android_base_ = "/data/data/" + pkg_name + "/cache";
// Check if application data directory exist. If not exist, usually means
we run tvm_rpc from
// adb shell terminal.
- if (stat(android_base_.data(), &statbuf) == -1 ||
!S_ISDIR(statbuf.st_mode)) {
+ if (!std::filesystem::is_directory(android_base_)) {
// Tmp directory is always writable for 'shell' user.
android_base_ = "/data/local/tmp";
}
base_ = android_base_ + "/rpc";
-
#elif !defined(_WIN32)
- char cwd[PATH_MAX];
- if (getcwd(cwd, sizeof(cwd))) {
- base_ = std::string(cwd) + "/rpc";
- } else {
+ std::error_code ec;
+ base_ = std::filesystem::current_path(ec).string() + "/rpc";
+ if (ec) {
base_ = "./rpc";
}
#else
base_ = "./rpc";
#endif
- mkdir(base_.c_str(), 0777);
+ std::filesystem::create_directories(base_);
Review Comment:
Use the `std::error_code` overload for `std::filesystem::create_directories`
to prevent unhandled exceptions from crashing the server if directory creation
fails.
```c
std::error_code ec;
std::filesystem::create_directories(base_, ec);
if (ec) {
LOG(WARNING) << "Failed to create directory " << base_ << ": " <<
ec.message();
}
```
##########
apps/cpp_rpc/rpc_env.cc:
##########
@@ -69,38 +57,41 @@ namespace tvm {
namespace runtime {
RPCEnv::RPCEnv(const std::string& wd) {
- if (wd != "") {
+ if (!wd.empty()) {
base_ = wd + "/.cache";
- mkdir(wd.c_str(), 0777);
- mkdir(base_.c_str(), 0777);
+ std::filesystem::create_directories(base_);
Review Comment:
Calling `std::filesystem::create_directories` without an `std::error_code`
argument will throw a `std::filesystem::filesystem_error` exception if
directory creation fails (e.g., due to permission issues or a read-only
filesystem). This will crash the RPC server.
It is safer to use the overload that accepts an `std::error_code` and handle
or log the error gracefully.
```c
std::error_code ec;
std::filesystem::create_directories(base_, ec);
if (ec) {
LOG(WARNING) << "Failed to create directory " << base_ << ": " <<
ec.message();
}
```
##########
apps/cpp_rpc/rpc_env.cc:
##########
@@ -69,38 +57,41 @@ namespace tvm {
namespace runtime {
RPCEnv::RPCEnv(const std::string& wd) {
- if (wd != "") {
+ if (!wd.empty()) {
base_ = wd + "/.cache";
- mkdir(wd.c_str(), 0777);
- mkdir(base_.c_str(), 0777);
+ std::filesystem::create_directories(base_);
} else {
#if defined(ANDROID) || defined(__ANDROID__)
- char cwd[PATH_MAX];
- auto cmdline = fopen("/proc/self/cmdline", "r");
- fread(cwd, 1, sizeof(cwd), cmdline);
- fclose(cmdline);
- std::string android_base_ = "/data/data/" + std::string(cwd) + "/cache";
- struct stat statbuf;
+ std::string pkg_name;
+ if (std::ifstream cmdline("/proc/self/cmdline"); cmdline) {
+ std::getline(cmdline, pkg_name, '\0');
+ }
+ std::string android_base_ = "/data/data/" + pkg_name + "/cache";
// Check if application data directory exist. If not exist, usually means
we run tvm_rpc from
// adb shell terminal.
- if (stat(android_base_.data(), &statbuf) == -1 ||
!S_ISDIR(statbuf.st_mode)) {
+ if (!std::filesystem::is_directory(android_base_)) {
// Tmp directory is always writable for 'shell' user.
android_base_ = "/data/local/tmp";
}
base_ = android_base_ + "/rpc";
-
#elif !defined(_WIN32)
- char cwd[PATH_MAX];
- if (getcwd(cwd, sizeof(cwd))) {
- base_ = std::string(cwd) + "/rpc";
- } else {
+ std::error_code ec;
+ base_ = std::filesystem::current_path(ec).string() + "/rpc";
+ if (ec) {
base_ = "./rpc";
}
#else
base_ = "./rpc";
#endif
- mkdir(base_.c_str(), 0777);
+ std::filesystem::create_directories(base_);
}
+ std::error_code ec;
+ std::filesystem::permissions(
+ base_,
+ std::filesystem::perms::all,
+ std::filesystem::perm_options::replace,
+ ec
+ );
Review Comment:
Explicitly setting the directory permissions to
`std::filesystem::perms::all` (0777) makes the RPC cache directory
world-writable. Since this directory is used to store and load compiled shared
libraries (`.so` files) via `tvm.rpc.server.load_module`, any local user could
replace these libraries with malicious ones, leading to arbitrary code
execution or local privilege escalation.
Instead of overriding the process's `umask` with `perms::all`, you should
either rely on the default permissions created by
`std::filesystem::create_directories` (which respects `umask`), or explicitly
set safer permissions such as `std::filesystem::perms::owner_all` (0700).
```suggestion
std::error_code ec;
std::filesystem::permissions(
base_,
std::filesystem::perms::owner_all,
std::filesystem::perm_options::replace,
ec
);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
