fix: retry force_rate hook patching - #128
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review: v1.12.12 — force_rate retry handling
Strengths
is_qzeroded_process()guard (force_rate.c:25–32): Correctly restricts the retry wait to the real qzeroded process, so bash wrappers and subshells that inheritLD_PRELOADstill bail out instantly. This keeps the non-qzeroded fast path clean.__builtin___clear_cacheaftermemcpy(force_rate.c:98): This is a correctness win. Writing x86 instructions viamemcpythen executing them can hit stale instruction-cache entries on some microarchitectures. Good catch.page_size <= 0guard (force_rate.c:70–71): Defensive, correct.sysconf(_SC_PAGESIZE)can return -1 on error; using that as the mincore length would be UB.- Diagnostic
fprintfon retry exhaustion (force_rate.c:85–88): Makes silent patch failures visible in server logs, which will help diagnose future timing issues.
Issues
Critical (Must Fix)
Committed prebuilt binary with no verification (force_rate.so)
A compiled .so is committed alongside the source. There is no build script, Makefile target, or CI step in this diff that rebuilds the binary from force_rate.c and verifies the result matches the committed file. This means:
- The committed binary could diverge from the source silently (accidental or deliberate).
- Anyone deploying this repo gets the binary; there is no way to confirm it was compiled from this exact source.
- Future PRs that edit
force_rate.cmay forget to rebuild, silently shipping stale code.
Fix: Build and hash-verify the .so in CI as part of the merge check, or generate it at install time. At minimum, add a make force_rate.so recipe and fail CI if the committed binary does not match a fresh build.
Important (Should Fix)
page_is_mapped() treats unexpected mincore errors as "mapped" (force_rate.c:35–39)
return errno != ENOMEM && errno != EINVAL;ENOMEM means the range is not mapped — correct to return 0. But errors like EFAULT (invalid vec pointer) or other unexpected codes return 1 (treated as "mapped"), and the code proceeds to mprotect + memcpy on an address that may not actually be accessible. This would likely segfault. The safer idiom is to return 0 on any mincore failure:
return 0; /* treat any error as not-yet-mapped */The only reason to distinguish errors here is to avoid false negatives on ENOMEM; every other error should be treated conservatively.
wait_for_target_page() blocks the dynamic linker for up to ~1 s (force_rate.c:42–53)
Constructor functions (__attribute__((constructor))) run on the linker's critical path. Blocking here for 50 × 20 ms delays dlopen/process startup. This is gated on is_qzeroded_process(), so non-qzeroded processes are unaffected — but any tool that wraps or execves qzeroded (e.g., a launcher that does pre-exec setup) will stall. Consider logging a single message at attempt 0 so the delay is visible in server output rather than appearing as a hang.
nanosleep ignores EINTR (force_rate.c:48)
A signal arriving during the 20 ms sleep causes nanosleep to return early with EINTR. The remaining sleep time is discarded, effectively shortening the retry window under signal load. For a game server constructor this is low-risk, but the standard pattern is:
struct timespec rem;
while (nanosleep(&delay, &rem) == -1 && errno == EINTR)
delay = rem;"TBD" PR number in release notes (docs/user/releases.md:7)
The releases.md entry has | TBD | for the PR link. This should be the actual PR number before merge so the changelog remains navigable.
Minor (Nice to Have)
is_qzeroded_process() uses a 256-byte path buffer (force_rate.c:27)
readlink("/proc/self/exe", buf, 255) silently truncates paths longer than 255 bytes. PATH_MAX is 4096 on Linux. In practice qzeroded's install path is short, but using PATH_MAX or a heap-allocated buffer is more correct. If the path truncates mid-name, strstr(exe_path, "qzeroded") could produce a false negative.
Off-by-one retry count (force_rate.c:44–52)
The loop runs 50 iterations, then wait_for_target_page calls page_is_mapped one more time unconditionally on return (force_rate.c:52). The effective attempt count is 51, not 50. The comment /* 20ms, 1s total */ is therefore slightly off (1.02 s max). Either fold the final check into the loop or update the comment.
PATCH_RETRY_DELAY_NS comment (force_rate.c:23)
#define PATCH_RETRY_DELAY_NS 20000000L /* 20ms, 1s total */The "1s total" claim belongs on PATCH_RETRY_ATTEMPTS, not the per-delay constant. Consider moving the combined-total comment to a single block above both defines.
Assessment
Ready to merge? No — with fixes
Reasoning: The retry logic and icache flush are correct and solve a real race condition, but committing a prebuilt binary without a reproducible build step is a supply-chain integrity problem that should be addressed before the code ships to production hosts. The page_is_mapped error-handling issue is also a latent crash risk worth fixing first.
|
Addressed the review findings:
Current verification:
|
1 participant
Summary
force_rate.sogives up when the target text page is not mapped yetLD_PRELOADwrapper/subprocess behavior quiet by skipping retries outside qzerodedforce_rate.soand bump release metadata to1.12.12make verify-system-hooksplus GitHub Actions coverage to rebuild and compare the committed hook binaryReview fixes
mincorefailure as not mapped instead of assuming unexpected errors are safenanosleep(..., EINTR)during retry delayPATH_MAXfor/proc/self/exedetectionTBDentry with PR fix: retry force_rate hook patching #128Test Plan
make force-rate.somake verify-system-hooksgcc -shared -fPIC -Wall -Wextra -Werror -o /tmp/force_rate.check.so ql-assets/data/system-hooks/force_rate.cLD_PRELOAD=/tmp/force_rate.check.so /bin/trueemits no outputpython -m json.tool docs/user/version.jsongit diff --checkpytest tests/test_ld_preload_paths.py tests/test_system_hooks_predicate.py tests/test_apply_hooks_preflight.py -q