Variable replacement and built ins#2
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| int ExitBuiltin::invoke(const std::vector<std::string>& args, ShellProcessContext& ctx) { | ||
| int code = 0; | ||
| if (args.size() > 1) { | ||
| code = std::atoi(args[1].c_str()); |
Check warning
Code scanning / devskim
These functions are historically error-prone and have been associated with a significant number of vulnerabilities. Most of these functions have safer alternatives, such as replacing 'strcpy' with 'strlcpy' or 'strcpy_s'. Warning
| std::vector<const char*> envp; | ||
| for (const auto& arg : env_map) { | ||
| envp.push_back((arg.first+ "=" + arg.second).c_str()); | ||
| envp.push_back((arg.first + "=" + arg.second).c_str()); |
Check failure
Code scanning / CodeQL
Use of string after lifetime ends High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, to fix this issue you must ensure that any const char* obtained from std::string::c_str() does not outlive the owning std::string. Here, we are building an environment array std::vector<const char*> envp from env_map. The current code uses a temporary concatenated std::string whose lifetime ends immediately; the resulting const char* becomes dangling. The safest solution without changing external behavior is to keep owning std::string objects alive in parallel with the const char* pointers, for at least as long as envp is used by the caller.
The best way to do this within the shown snippet is to change convertEnvironment so that it returns a std::vector<std::string> of key=value strings instead of std::vector<const char*>, leaving the responsibility for creating const char* arrays with stable storage to the caller. However, since we must not assume or modify code outside this file, we should instead create an internal static storage for the environment strings that outlives the returned envp vector. Specifically, inside convertEnvironment, we can build a static thread_local std::vector<std::string> of key=value strings, clear and repopulate it every call, and then fill envp with c_str() pointers into that vector. The thread_local lifetime guarantees that the strings remain valid until thread termination, covering any subsequent use of envp in this thread. We must also ensure that the terminating nullptr remains as-is.
Concretely:
- Inside
PlatformExecutionPolicy::convertEnvironment, introduce astatic thread_local std::vector<std::string> env_storage;. - At the start of the function, clear
env_storage. - In the loop, construct each
key=valuestring as astd::stringstored intoenv_storage(e.g.,env_storage.emplace_back(arg.first + "=" + arg.second);) and then pushenv_storage.back().c_str()intoenvp. - Maintain the final
envp.push_back(nullptr);and the return typestd::vector<const char*>unchanged. - No new headers are needed;
<vector>and<string>are already included.
This keeps functionality the same (still returns std::vector<const char*> with a terminating nullptr) while ensuring the underlying strings outlive the returned pointers.
| @@ -133,9 +133,16 @@ | ||
| auto current_env = EnvironmentCache::instance().get_all(); | ||
| env_map.insert(current_env.begin(), current_env.end()); | ||
| } | ||
| // Storage for "KEY=VALUE" strings whose c_str() pointers are returned. | ||
| // thread_local ensures the strings outlive this function call within the thread. | ||
| static thread_local std::vector<std::string> env_storage; | ||
| env_storage.clear(); | ||
|
|
||
| std::vector<const char*> envp; | ||
| envp.reserve(env_map.size() + 1); // +1 for terminating nullptr | ||
| for (const auto& arg : env_map) { | ||
| envp.push_back((arg.first + "=" + arg.second).c_str()); | ||
| env_storage.emplace_back(arg.first + "=" + arg.second); | ||
| envp.push_back(env_storage.back().c_str()); | ||
| } | ||
| envp.push_back(nullptr); // NULL-terminated | ||
|
|
| std::cerr << "kill: missing pid" << std::endl; | ||
| return 1; | ||
| } | ||
| int pid = std::atoi(args[1].c_str()); |
Check warning
Code scanning / devskim
These functions are historically error-prone and have been associated with a significant number of vulnerabilities. Most of these functions have safer alternatives, such as replacing 'strcpy' with 'strlcpy' or 'strcpy_s'. Warning
| } | ||
|
|
||
| std::optional<std::string> get_home_directory() { | ||
| const char* home = getenv("HOME"); |
Check warning
Code scanning / devskim
These functions are historically error-prone and have been associated with a significant number of vulnerabilities. Most of these functions have safer alternatives, such as replacing 'strcpy' with 'strlcpy' or 'strcpy_s'. Warning
modified: src/lib/executor/executor_win32.cpp
| variables_{}, | ||
| output_(output), | ||
| error_output_(error_output), builtins_{}, history_{} {} | ||
| explicit ShellInterpreter(wshell::IOutputDestination& output, |
Check warning
Code scanning / PREfast
Variable 'wshell::ShellInterpreterwshell::PlatformExecutionPolicy::process_context_' is uninitialized. Always initialize a member variable (type.6). Warning
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
2 participants
First pass at variable replacement and some built ins are done.