fix: use pthread_once for thread-safe getenv initialization#609
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a data race in init_real_getenv() by switching to pthread_once() for one-time, thread-safe initialization of the dlsym()-resolved getenv function pointer.
Changes:
- Replace manual
real_getenv == NULLcheck withpthread_once()to guarantee single initialization. - Split initialization into
init_real_getenv_once()(doesdlsym) andinit_real_getenv()(wrapper). - Update failure messaging/comment and add a CodeQL source root marker file.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| containers/agent/one-shot-token/one-shot-token.c | Uses pthread_once() to make real_getenv initialization thread-safe and adjusts fatal error handling. |
| _codeql_detected_source_root | Adds a CodeQL-detected source root marker. |
Comments suppressed due to low confidence (1)
_codeql_detected_source_root:1
- This looks like an auto-generated CodeQL marker file. If it’s generated by tooling during analysis, it typically shouldn’t be committed; consider removing it and adding it to
.gitignore(or documenting why it must be tracked if your pipeline relies on it being present in-repo).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static void init_real_getenv_once(void) { | ||
| real_getenv = dlsym(RTLD_NEXT, "getenv"); | ||
| if (real_getenv == NULL) { | ||
| real_getenv = dlsym(RTLD_NEXT, "getenv"); | ||
| if (real_getenv == NULL) { | ||
| fprintf(stderr, "[one-shot-token] ERROR: Could not find real getenv: %s\n", dlerror()); | ||
| /* Fall back to a no-op to prevent crash */ | ||
| abort(); | ||
| } | ||
| fprintf(stderr, "[one-shot-token] FATAL: Could not find real getenv: %s\n", dlerror()); | ||
| /* Cannot recover - abort to prevent undefined behavior */ | ||
| abort(); | ||
| } |
There was a problem hiding this comment.
dlsym() errors should be checked via dlerror(), not by comparing the returned pointer to NULL. Per dlsym() semantics, NULL can be a valid symbol value, and dlerror() may also return a stale error unless you clear it before calling dlsym(). Clear dlerror() first, call dlsym(), then call dlerror() and abort only if it reports an error.
See below for a potential fix:
const char *err;
/* Clear any existing error */
(void)dlerror();
real_getenv = dlsym(RTLD_NEXT, "getenv");
err = dlerror();
if (err != NULL) {
fprintf(stderr, "[one-shot-token] FATAL: Could not find real getenv: %s\n", err);
|
|
||
| /* Initialize the real getenv pointer (called exactly once via pthread_once) */ | ||
| static void init_real_getenv_once(void) { | ||
| real_getenv = dlsym(RTLD_NEXT, "getenv"); |
There was a problem hiding this comment.
Assigning the void * result of dlsym() directly to a function pointer is not portable ISO C and can trigger warnings (even if it works on many POSIX platforms). Prefer assigning via an intermediate void * and memcpy, or the common POSIX pattern *(void **)(&real_getenv) = dlsym(...); to avoid undefined-behavior concerns and compiler diagnostics.
See below for a potential fix:
void *sym = dlsym(RTLD_NEXT, "getenv");
if (sym == NULL) {
fprintf(stderr, "[one-shot-token] FATAL: Could not find real getenv: %s\n", dlerror());
/* Cannot recover - abort to prevent undefined behavior */
abort();
}
memcpy(&real_getenv, &sym, sizeof(real_getenv));
…ss (#604) * feat: add one-shot token LD_PRELOAD library for single-use token access Adds an LD_PRELOAD library that intercepts getenv() calls for sensitive GitHub token environment variables. On first access, returns the real value and immediately unsets the variable, preventing subsequent reads by malicious code. Protected tokens: COPILOT_GITHUB_TOKEN, GITHUB_TOKEN, GH_TOKEN, GITHUB_API_TOKEN, GITHUB_PAT, GH_ACCESS_TOKEN - Add one-shot-token.c with getenv interception logic - Build library in Dockerfile during image build - Enable LD_PRELOAD in entrypoint for both container and chroot modes - Add documentation explaining the mechanism and security properties * fix: improve one-shot token library copy robustness in chroot mode (#606) * Initial plan * fix: improve one-shot token library copy with better error handling Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * Update containers/agent/one-shot-token/README.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update containers/agent/one-shot-token/README.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: use pthread_once for thread-safe getenv initialization (#609) * Initial plan * fix: use pthread_once for thread-safe getenv initialization Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * chore: complete thread safety fix Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * test: add integration tests for one-shot token LD_PRELOAD library (#608) * Initial plan * test: add integration tests for one-shot token LD_PRELOAD library Add comprehensive integration tests that verify the one-shot token protection mechanism works correctly in both container and chroot modes. Tests verify: - Protected tokens (GITHUB_TOKEN, COPILOT_GITHUB_TOKEN, OPENAI_API_KEY) can be read once - Subsequent reads return empty/null (token has been cleared) - Non-sensitive environment variables are not affected - Multiple tokens are handled independently - Behavior works with both shell (printenv) and programmatic (Python getenv) access - Edge cases (empty values, nonexistent tokens, special characters) are handled Tests address feedback from PR #604 review requesting integration tests for the one-shot token feature to prevent regressions. Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: preserve secure_getenv semantics in one-shot token interposer (#610) * Initial plan * fix: implement proper secure_getenv with dlsym fallback Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: add thread safety to secure_getenv initialization Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Co-authored-by: Landon Cox <landon.cox@microsoft.com> * feat: add runtime configuration for one-shot token protection via AWF_ONE_SHOT_TOKENS (#607) * Initial plan * feat: add runtime configuration for one-shot token list via AWF_ONE_SHOT_TOKENS Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: address code review feedback for one-shot token library - Make initialization thread-safe using existing mutex - Add cleanup of allocated tokens on memory allocation failure - Use isspace() for comprehensive whitespace trimming (handles newlines, tabs, etc.) Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: resolve race condition and buffer underflow in token parsing - Fix potential buffer underflow when trimming empty strings - Fix race condition by keeping mutex held during get_token_index() call - Ensure thread-safe access to token list during lookup Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * refactor: optimize strlen usage and add MAX_TOKENS documentation - Cache strlen result to avoid redundant computation - Add inline comment explaining MAX_TOKENS limit rationale Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * style: improve comment formatting for MAX_TOKENS Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: use strtok_r and fallback to defaults for empty token list - Replace strtok() with strtok_r() to avoid interfering with application code - Fall back to default token list if AWF_ONE_SHOT_TOKENS parses to zero tokens - Add warning messages when falling back due to misconfiguration Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * docs: update README with fallback behavior and strtok_r details Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * refactor: explicitly reset num_tokens in fallback path for clarity Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * docs: clarify defensive programming intent in num_tokens reset Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Co-authored-by: Landon Cox <landon.cox@microsoft.com> * fix: add missing secure_getenv interposer initialization (#619) * Initial plan * fix: add missing secure_getenv declarations and initialization Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * chore: remove build artifacts from git Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * docs: update progress - fix complete Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * chore: remove test files from git Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes data race in
init_real_getenv()where multiple threads could observereal_getenv == NULLand calldlsym()concurrently, writing to the function pointer simultaneously.Changes:
pthread_once()for guaranteed single initializationinit_real_getenv_once()(initialization logic) andinit_real_getenv()(pthread_once wrapper)Before:
After:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.