Repository navigation
Preventing fault-injection and sampler perf counting from leaking into production build - #832
zhengyu123 wants to merge 4 commits into
Conversation
CI Test ResultsRun: #37464127402 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-06 12:55:31 UTC |
zhengyu123
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca11b8383a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
This comment has been minimized.
This comment has been minimized.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Thanks, this is a useful guard. I checked the markers against the source: faultinj:: and the counter names exist only under the flag defines, crashNow() also only under DEBUG (which release doesn't define), and --strip-debug keeps .symtab. I didn't find a way for a flagged build to slip through today. My inline comments are mostly about the misleading strip comment, how much detection there really is, and test gaps.
One related point that isn't in the diff, so I can't comment on it inline: ConfigurationPresets.kt:46 and :56 use project.hasProperty("enableFaultInjection") / hasProperty("enableSamplerPerf"). That is true even for -PenableFaultInjection=false, or for enableFaultInjection=false in gradle.properties or ORG_GRADLE_PROJECT_* env vars. So someone who explicitly turns the flag off still gets -D__FAULT_INJECTION__. That trap is a likely way for these flags to end up on in a release by accident. I'd suggest (maybe as a follow-up) parsing the value as a boolean, and failing the release build at Gradle configuration time if either flag is set. This script would stay as the last line of defence after the full compile.
| # e.g. check-release-flags.sh libs/linux-x64/libjavaProfiler.so | ||
| # | ||
| # Relies on the release .so still carrying its symbol table: build.sh strips | ||
| # only debug sections (--strip-debug), not the symbol table itself, before |
There was a problem hiding this comment.
This (and the error text at line 68-69, "see build.sh") says build.sh does the --strip-debug, but build.sh doesn't strip anything. The strip comes from Gradle's NativeLinkTask.stripLibrary (strip --strip-debug on Linux, strip -S on macOS). The whole guard depends on .symtab surviving. So whoever later changes that task to --strip-all/--strip-unneeded needs to find this dependency, and this comment sends them to the wrong file. Could you point it at NativeLinkTask.kt instead? Ideally also add a back-reference comment next to stripLibrary saying this check relies on the symtab.
| SYMBOL_MARKERS=( | ||
| '_ZN8faultinj::-PenableFaultInjection::the faultinj:: namespace (faultInjection.h)' | ||
| '_Z8crashNowv::-PenableFaultInjection::crashNow() (faultInjection.h)' | ||
| '_ZN16SamplerPerfProbe::-PenableSamplerPerf::the SamplerPerfProbe class (samplerPerf.h)' |
There was a problem hiding this comment.
I doubt this marker ever fires on a real -O3 release build. SamplerPerfProbe is header-only, with an inline ctor/dtor, so I wouldn't expect any out-of-line _ZN16SamplerPerfProbe* symbol to be emitted. The test fixture's _ZN16SamplerPerfProbeD2Ev (t) models something a real build probably doesn't produce. In practice that leaves the sampler_ticks./sampler_count. strings as the only real detection for sampler-perf, while the tests suggest two independent layers. Could you check nm on an actual -PenableSamplerPerf release .so? If the symbol isn't there, either drop the marker or replace it with something that's guaranteed to survive.
| # constructor and destructor without needing c++filt. | ||
| SYMBOL_MARKERS=( | ||
| '_ZN8faultinj::-PenableFaultInjection::the faultinj:: namespace (faultInjection.h)' | ||
| '_Z8crashNowv::-PenableFaultInjection::crashNow() (faultInjection.h)' |
There was a problem hiding this comment.
This marker isn't tested on its own. The nm-faultinj fixture has crashNow and three _ZN8faultinj symbols, so deleting or mistyping this entry leaves the suite green. Yet it's exactly the marker that would catch a build where the faultinj:: helpers got inlined or gc-section'd away. Could you add a fixture with only _Z8crashNowv?
| 'sampler_count.::-PenableSamplerPerf::the "sampler_count.*" counters (counters.h)' | ||
| ) | ||
|
|
||
| SYMTAB=$("${NM}" "${SO}" 2>/dev/null) || die "${NM} failed on ${SO}" |
There was a problem hiding this comment.
Sending nm's stderr to /dev/null drops the useful part when it fails, e.g. "file format not recognized" from a host-only nm on an aarch64 .so, or a truncated file. CI would then show just "nm failed on …". I'd let stderr through, or capture it and include it in the die message.
| exit 1 | ||
| fi | ||
|
|
||
| STRTAB=$("${STRINGS}" "${SO}") || die "${STRINGS} failed on ${SO}" |
There was a problem hiding this comment.
This path isn't tested. The stub defines FAKE_STRINGS_FAIL (test line 33), but no test case sets it. The command -v "tool not found" guards (lines 48-49) aren't covered either. Since the string markers are effectively the only reliable sampler-perf detection (see my comment on line 56), a regression that turned a strings failure into an empty STRTAB would silently disable them. Worth adding a test case for both.
| fi | ||
| done | ||
|
|
||
| for entry in "${STRING_MARKERS[@]}"; do |
There was a problem hiding this comment.
Nit: this loop is a copy of the one above. The parsing, grep and FAIL message are the same; only the array and the text searched differ. A small check_markers "$haystack" "${ARR[@]}" helper would keep the two from drifting apart, e.g. if one ever loses the SIGPIPE-safe here-string. Minor, too: each here-string re-copies the multi-MB nm/strings output, six times in total. Writing each output to a file once and running a single grep -F -f would avoid that. Not a big deal at CI scale.
kaahos
left a comment
kaahos
left a comment
There was a problem hiding this comment.
thanks for the work! Apart from the comments left by Roman, I have nothing else to report. Please address them first, but otherwise this looks good to me!
| SYMBOL_MARKERS=( | ||
| '_ZN8faultinj::-PenableFaultInjection::the faultinj:: namespace (faultInjection.h)' | ||
| '_Z8crashNowv::-PenableFaultInjection::crashNow() (faultInjection.h)' | ||
| '_ZN16SamplerPerfProbe::-PenableSamplerPerf::the SamplerPerfProbe class (samplerPerf.h)' | ||
| ) |
There was a problem hiding this comment.
Related to @rkennke's check of the markers against the source: right now that check only happens by hand. Nothing keeps these arrays in sync with the code they detect, or with the list of flags:
- New flags aren't covered: if someone adds a 3rd
hasProperty("enable…")next toConfigurationPresets.kt:56, this script never checks for it, and a release build with that flag passes. - Renames are unnoticed: the test fixtures (
check_release_flags_test.sh:52-83) are copies of these markers; the suite never reads the source. Example: if"sampler_ticks.cpu"incounters.his renamed, the suite stays green while sampler-perf detection falls back to the compiler-dependentSamplerPerfProbesymbol, or to nothing.
This branch has not been deployed
3 participants
What does this PR do?:
Adds a release-build guard,
.gitlab/scripts/check-release-flags.sh. It fails the build iflibjavaProfiler.sowas compiled with either of the opt-in flags-PenableFaultInjectionor-PenableSamplerPerf.build.shruns it on every target, right after copying the native libs intolibs/and before the ABI floor check.The script looks for markers that only exist when one of the flags is on:
nm, mangled names, so every overload, constructor and destructor matches):_ZN8faultinj(thefaultinj::namespace)_Z8crashNowv(crashNow())_ZN16SamplerPerfProbe(theSamplerPerfProbeclass)strings): thefaults_injected,sampler_ticks.*andsampler_count.*counter names.Counters::describeCounters()keeps these in.rodata, so the check still works when the symbols get inlined away.If
nmfinds no symbols, or fails, the script reports an error rather than a pass, so a wrong path or an unreadable file can't slip through.Motivation:
Both flags are additive: they add
-D__FAULT_INJECTION__/-D__SAMPLER_PERF__on top of the normal release config. A build made with either one still links, runs and passes every existing check. Fault injection deliberately corrupts memory reads on a random sample of calls, and sampler-perf adds probes and a timing report atProfiler::stop(). Shipping either to customers would be a silent correctness or performance regression, and nothing in the pipeline caught it before this PR.Additional Notes:
SamplerPerf(withoutProbe) is in every build, since its disabled variant ships by default. The clean test fixture includes it to show the check doesn't over-match..sokeeping its symbol table. The Gradle link task (NativeLinkTask.kt) only runsstrip --strip-debug, so the table survives. (The script comment saysbuild.shdoes the stripping; it's actually the Gradle task. That's a small comment fix.)crashNow()is also compiled intoDEBUGbuilds, but release builds don't defineDEBUG.NM/STRINGScan be overridden, which lets the unit tests feed in canned output.How to test the change?:
.gitlab/scripts/tests/check_release_flags_test.shuse stubnm/stringstools, so they don't need a compiler or a real flagged build. They cover:bash -n+shellcheck+ run), alongside the ABI floor tests.bash .gitlab/scripts/tests/check_release_flags_test.sh-PenableFaultInjection(or-PenableSamplerPerf) and run.gitlab/scripts/check-release-flags.sh ddprof-lib/build/.../libjavaProfiler.so. It should fail, and a normal release build should pass.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!