Stop remoted from writing the sender counter into agent 0 rids on key… - #2303
Merged
Merged
Conversation
… reload. Keep the outbound counter on the keystore instead of keyentries[keysize], and replace the sleep(1) FreeKeys drain with an rwlock so a reload cannot close agent files while send still holds them.
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes issue #2065 by isolating the outbound (sender) counter from keyentries[keysize] and replacing the reload “sleep-to-drain” approach with an rwlock to prevent reload/free from racing concurrent send/lookup paths.
Changes:
- Store/reload the sender counter via a dedicated
keystore.sender_fp/sender_inodeinstead ofkeyentries[keysize]. - Introduce a keystore rwlock (read for lookup/send, write for reload) and update remoted call sites to hold the read lock across key usage.
- Add a regression test for the sender-counter overwrite race and wire it into the regressions Makefile.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/regressions/issue_2065_sender_counter.c | Adds regression coverage ensuring sender counter persistence does not overwrite agent 0’s rids during reload/free windows. |
| src/tests/regressions/Makefile | Builds and registers the new regression binary. |
| src/shared/pthreads_op.c | Adds wrapper helpers for pthread rwlocks with consistent error handling. |
| src/headers/pthreads_op.h | Exposes the new rwlock wrapper APIs. |
| src/remoted/sendmsg.c | Replaces key update mutex with rwlock and adds bounds checks requiring callers to hold the key read lock. |
| src/remoted/secure.c | Holds the key read lock across lookup/decrypt/usage paths and releases on all early-continues. |
| src/remoted/manager.c | Wraps send paths with key read locks; avoids holding locks across sleeps and long file transfers; snapshots src IP safely. |
| src/remoted/ar-forward.c | Updates key lock usage to the new read-lock API. |
| src/remoted/remoted.h | Updates the public remoted key-lock API to read/write rwlock functions. |
| src/os_crypto/shared/msgs.c | Moves sender counter to dedicated keystore fields; adds sender reload/close helpers; updates counter start logic. |
| src/os_crypto/shared/keys.c | Removes extra “sender entry” slot; removes sleep-based drain; preserves some agent runtime state across reload. |
| src/headers/sec.h | Extends keystore with sender-counter file state and exports sender-counter APIs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| merror("Unable to open agent file. errno: %d", my_error); | ||
| ErrorExit(FOPEN_ERROR, __local_name, rids_file, errno, strerror(errno)); |
Comment on lines
+121
to
129
| debug2("Assigning counter for agent %s: '%u:%u'.", | ||
| keys->keyentries[i]->name, g_c, l_c); | ||
|
|
||
| keys->keyentries[i]->global = g_c; | ||
| keys->keyentries[i]->local = l_c; | ||
| } | ||
| keys->keyentries[i]->global = g_c; | ||
| keys->keyentries[i]->local = l_c; | ||
| } | ||
|
|
||
| /* Initialize mutex */ | ||
| pthread_mutex_init(&keys->keyentries[i]->mutex, NULL); | ||
| keys->keyentries[i]->inode = File_Inode(rids_file); | ||
| } |
Comment on lines
+196
to
+210
| /* Store sender counter on the dedicated keystore slot, never agent rids. */ | ||
| void OS_StoreSenderCounter(const keystore *keys, unsigned int global, unsigned int local) | ||
| { | ||
| if (!keys->sender_fp) { | ||
| return; | ||
| } | ||
|
|
||
| /* Write to the beginning of the file */ | ||
| fseek(keys->keyentries[keys->keysize]->fp, 0, SEEK_SET); | ||
| fprintf(keys->keyentries[keys->keysize]->fp, "%u:%u:", global, local); | ||
| fflush(keys->keyentries[keys->keysize]->fp); | ||
| if (fseek(keys->sender_fp, 0, SEEK_SET) != 0) { | ||
| merror("Unable to seek sender counter: %s (%d)", strerror(errno), errno); | ||
| return; | ||
| } | ||
| fprintf(keys->sender_fp, "%u:%u:", global, local); | ||
| fflush(keys->sender_fp); | ||
| } |
Comment on lines
287
to
+305
| hashid = keys->keyhash_id; | ||
| haship = keys->keyhash_ip; | ||
|
|
||
| /* Zero the entries */ | ||
| keys->keysize = 0; | ||
| keys->keyhash_id = NULL; | ||
| keys->keyhash_ip = NULL; | ||
|
|
||
| /* Sleep to give time to other threads to stop using them */ | ||
| sleep(1); | ||
|
|
||
| /* Free the hashes */ | ||
| OSHash_Free(hashid); | ||
| OSHash_Free(haship); | ||
| if (hashid) { | ||
| OSHash_Free(hashid); | ||
| } | ||
| if (haship) { | ||
| OSHash_Free(haship); | ||
| } | ||
|
|
||
| if (keys->sender_fp) { | ||
| OS_CloseSenderCounter(keys); | ||
| } | ||
|
|
||
| for (i = 0; i <= _keysize; i++) { | ||
| for (i = 0; i < _keysize; i++) { |
Keep outbound counters across a bad sender file parse, re-resolve agent IDs after dropping the key lock, and serialize the public store path so close/reload cannot write a closed FILE*.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… reload.
Keep the outbound counter on the keystore instead of keyentries[keysize], and replace the sleep(1) FreeKeys drain with an rwlock so a reload cannot close agent files while send still holds them. Closes issue #2065