Repository navigation
Preventing fault-injection and sampler perf counting from leaking into production build #832
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| #! /bin/bash | ||
| # | ||
| # Checks that a release-build shared object carries no trace of the opt-in | ||
| # -PenableFaultInjection / -PenableSamplerPerf build flags (see | ||
| # ConfigurationPresets.kt). Both are additive: they append -D__FAULT_INJECTION__ | ||
| # / -D__SAMPLER_PERF__ on top of the normal release config, so a build with | ||
| # either one active still passes every other check -- it links, it runs, it | ||
| # looks like a release build. Fault injection deliberately corrupts memory | ||
| # reads on a random sample of calls, and the sampler-perf probes add a timing | ||
| # report at Profiler::stop(); shipping either to customers would be a silent | ||
| # correctness/perf regression that nothing else here would catch. | ||
| # | ||
| # Rather than diffing against a golden symbol list (which the additive nature | ||
| # of the flags would make brittle), this looks for markers that exist only | ||
| # because a flag was on: mangled symbols from the flag-only code (faultInjection.h | ||
| # / samplerPerf.h), plus the counter names those flags add to counters.h, which | ||
| # survive as plain strings in .rodata even if the symbols themselves get | ||
| # inlined away. | ||
| # | ||
| # Usage: check-release-flags.sh <shared-object> | ||
| # 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 | ||
|
Contributor
There was a problem hiding this comment. This (and the error text at line 68-69, "see build.sh") says |
||
| # this runs. | ||
| # | ||
| # Set NM / STRINGS to point at different binutils tools; the unit tests use | ||
| # them to feed canned output in. | ||
|
|
||
| set -eo pipefail | ||
|
|
||
| SO="$1" | ||
| NM="${NM:-nm}" | ||
| STRINGS="${STRINGS:-strings}" | ||
|
|
||
| die() { | ||
| echo "ERROR: $*" >&2 | ||
| exit 1 | ||
| } | ||
|
|
||
| if [ -z "${SO}" ]; then | ||
| echo "usage: $(basename "$0") <shared-object>" >&2 | ||
| exit 2 | ||
| fi | ||
|
|
||
| [ -f "${SO}" ] || die "no such file: ${SO}" | ||
| command -v "${NM}" >/dev/null 2>&1 || die "${NM} not found" | ||
| command -v "${STRINGS}" >/dev/null 2>&1 || die "${STRINGS} not found" | ||
|
|
||
| # "<marker>::<flag>::<description>". Symbol markers are substrings of the | ||
| # Itanium mangling nm prints by default, so they match every overload, | ||
| # constructor and destructor without needing c++filt. | ||
| SYMBOL_MARKERS=( | ||
| '_ZN8faultinj::-PenableFaultInjection::the faultinj:: namespace (faultInjection.h)' | ||
| '_Z8crashNowv::-PenableFaultInjection::crashNow() (faultInjection.h)' | ||
|
Contributor
There was a problem hiding this comment. This marker isn't tested on its own. The |
||
| '_ZN16SamplerPerfProbe::-PenableSamplerPerf::the SamplerPerfProbe class (samplerPerf.h)' | ||
|
Contributor
There was a problem hiding this comment. I doubt this marker ever fires on a real |
||
| ) | ||
|
Comment on lines
+53
to
+57
Contributor
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:
|
||
|
|
||
| STRING_MARKERS=( | ||
| 'faults_injected::-PenableFaultInjection::the "faults_injected" counter (counters.h)' | ||
| 'sampler_ticks.::-PenableSamplerPerf::the "sampler_ticks.*" counters (counters.h)' | ||
| 'sampler_count.::-PenableSamplerPerf::the "sampler_count.*" counters (counters.h)' | ||
| ) | ||
|
|
||
| SYMTAB=$("${NM}" "${SO}" 2>/dev/null) || die "${NM} failed on ${SO}" | ||
|
Contributor
There was a problem hiding this comment. Sending nm's stderr to |
||
| if [ -z "${SYMTAB}" ]; then | ||
| echo "ERROR: ${NM} found no symbols in ${SO}." >&2 | ||
| echo " A release build's symbol table survives strip --strip-debug (see" >&2 | ||
| echo " build.sh), so this is far more likely to mean the wrong file was" >&2 | ||
| echo " passed than that the library is clean. Refusing to report a pass." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| STRTAB=$("${STRINGS}" "${SO}") || die "${STRINGS} failed on ${SO}" | ||
|
Contributor
There was a problem hiding this comment. This path isn't tested. The stub defines |
||
|
|
||
| STATUS=0 | ||
|
|
||
| for entry in "${SYMBOL_MARKERS[@]}"; do | ||
| marker="${entry%%::*}" | ||
| rest="${entry#*::}" | ||
| flag="${rest%%::*}" | ||
| desc="${rest#*::}" | ||
| # A here-string, not a pipe: `printf ... | grep -q` would let grep's early | ||
| # exit on the first match send SIGPIPE back to printf, and pipefail would | ||
| # then report that SIGPIPE as the pipeline's failure -- turning a match | ||
| # into a false "not found". | ||
| if grep -qF -- "${marker}" <<< "${SYMTAB}"; then | ||
| echo "FAIL: ${SO} contains ${desc}, built only under ${flag}." >&2 | ||
| STATUS=1 | ||
| fi | ||
| done | ||
|
|
||
| for entry in "${STRING_MARKERS[@]}"; do | ||
|
Contributor
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 |
||
| marker="${entry%%::*}" | ||
| rest="${entry#*::}" | ||
| flag="${rest%%::*}" | ||
| desc="${rest#*::}" | ||
| if grep -qF -- "${marker}" <<< "${STRTAB}"; then | ||
| echo "FAIL: ${SO} contains ${desc}, built only under ${flag}." >&2 | ||
| STATUS=1 | ||
| fi | ||
| done | ||
|
|
||
| if [ "${STATUS}" -eq 0 ]; then | ||
| echo "OK: ${SO} carries no -PenableFaultInjection / -PenableSamplerPerf markers" | ||
| else | ||
| echo " Rebuild the release artifact without these -P flags." >&2 | ||
| fi | ||
|
|
||
| exit "${STATUS}" | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| #! /bin/bash | ||
| # Minimal, dependency-free unit tests for .gitlab/scripts/check-release-flags.sh. | ||
| # Run with: bash .gitlab/scripts/tests/check_release_flags_test.sh | ||
| # | ||
| # The checker reads the artifact through $NM/$STRINGS, so these tests supply | ||
| # stubs that print canned output. That keeps them runnable without a compiler | ||
| # or a real shared object built with -PenableFaultInjection / -PenableSamplerPerf. | ||
|
|
||
| # The run_* helpers are invoked indirectly, as arguments to the assert_* | ||
| # helpers, which shellcheck cannot see. | ||
| # shellcheck disable=SC2329 | ||
|
|
||
| set -eo pipefail | ||
|
|
||
| HERE=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) | ||
| # Overridable so the checker's guards can be mutation-checked against this suite. | ||
| CHECKER="${CHECKER:-${HERE}/../check-release-flags.sh}" | ||
|
|
||
| FAILED=0 | ||
| WORK=$(mktemp -d) | ||
| trap 'rm -rf "${WORK}"' EXIT | ||
|
|
||
| # A stub nm: prints ${FAKE_NM}. A stub strings: prints ${FAKE_STRINGS}. | ||
| cat > "${WORK}/nm" <<'STUB' | ||
| #! /bin/bash | ||
| [ -n "${FAKE_NM_FAIL}" ] && exit 1 | ||
| cat "${FAKE_NM}" | ||
| STUB | ||
| chmod +x "${WORK}/nm" | ||
|
|
||
| cat > "${WORK}/strings" <<'STUB' | ||
| #! /bin/bash | ||
| [ -n "${FAKE_STRINGS_FAIL}" ] && exit 1 | ||
| cat "${FAKE_STRINGS}" | ||
| STUB | ||
| chmod +x "${WORK}/strings" | ||
|
|
||
| : > "${WORK}/libjavaProfiler.so" | ||
|
|
||
| # A plain release build's symbol table: real symbols, none of the flag-only | ||
| # markers. SamplerPerf itself (unlike SamplerPerfProbe) always exists -- its | ||
| # disabled variant is what a build with neither flag ships -- so it belongs | ||
| # in the clean fixture, to prove the checker doesn't over-match on the name. | ||
| cat > "${WORK}/nm-clean" <<'EOF' | ||
| 0000000000064500 T Agent_OnLoad | ||
| 000000000006db10 t _ZN11SamplerPerf10primeClockEv | ||
| 000000000006db20 t _ZN11SamplerPerf6reportEv | ||
| 0000000000023000 t _ZN8Profiler4stopEv | ||
| EOF | ||
|
|
||
| # Mangled (Itanium) form, as plain nm -- not nm -C -- actually prints it. | ||
| cat > "${WORK}/nm-faultinj" <<'EOF' | ||
| 0000000000064500 T Agent_OnLoad | ||
| 000000000004b940 t _Z8crashNowv | ||
| 000000000004b9b0 t _ZN8faultinj10shouldFireEyPKc | ||
| 000000000004ba70 t _ZN8faultinj13poisonAddressEv | ||
| 000000000004b950 t _ZN8faultinj4initEv | ||
| EOF | ||
|
|
||
| cat > "${WORK}/nm-samplerperf" <<'EOF' | ||
| 0000000000064500 T Agent_OnLoad | ||
| 000000000006db10 t _ZN11SamplerPerf10primeClockEv | ||
| 000000000006db20 t _ZN11SamplerPerf6reportEv | ||
| 0000000000019ff0 t _ZN16SamplerPerfProbeD2Ev | ||
| EOF | ||
|
|
||
| : > "${WORK}/nm-empty" | ||
|
|
||
| cat > "${WORK}/strings-clean" <<'EOF' | ||
| method_resolution_dropped_tls | ||
| metadata_tree_null_child | ||
| EOF | ||
|
|
||
| cat > "${WORK}/strings-faultinj" <<'EOF' | ||
| method_resolution_dropped_tls | ||
| faults_injected | ||
| EOF | ||
|
|
||
| cat > "${WORK}/strings-samplerperf" <<'EOF' | ||
| method_resolution_dropped_tls | ||
| sampler_ticks.cpu | ||
| sampler_count.cpu | ||
| EOF | ||
|
|
||
| run_checker() { | ||
| local nm="$1" strings="$2" | ||
| FAKE_NM="${WORK}/${nm}" FAKE_STRINGS="${WORK}/${strings}" \ | ||
| NM="${WORK}/nm" STRINGS="${WORK}/strings" \ | ||
| "${CHECKER}" "${WORK}/libjavaProfiler.so" > "${WORK}/out" 2>&1 | ||
| } | ||
|
|
||
| # Invokes the checker with arguments verbatim, for the argument-handling cases. | ||
| run_raw() { | ||
| env FAKE_NM="${WORK}/nm-clean" FAKE_STRINGS="${WORK}/strings-clean" \ | ||
| NM="${WORK}/nm" STRINGS="${WORK}/strings" "${CHECKER}" "$@" > "${WORK}/out" 2>&1 | ||
| } | ||
|
|
||
| run_nm_unreadable() { | ||
| FAKE_NM_FAIL=1 FAKE_STRINGS="${WORK}/strings-clean" \ | ||
| NM="${WORK}/nm" STRINGS="${WORK}/strings" \ | ||
| "${CHECKER}" "${WORK}/libjavaProfiler.so" > "${WORK}/out" 2>&1 | ||
| } | ||
|
|
||
| assert_exit_code() { | ||
| local desc="$1" expected="$2"; shift 2 | ||
| local actual=0 | ||
| "$@" || actual=$? | ||
| if [ "${actual}" -eq "${expected}" ]; then | ||
| echo "PASS: ${desc}" | ||
| else | ||
| echo "FAIL: ${desc} — expected exit ${expected}, got ${actual}" | ||
| sed 's/^/ /' "${WORK}/out" | ||
| FAILED=1 | ||
| fi | ||
| } | ||
|
|
||
| assert_pass() { | ||
| local desc="$1"; shift | ||
| if "$@"; then | ||
| echo "PASS: ${desc}" | ||
| else | ||
| echo "FAIL: ${desc} — expected the check to pass" | ||
| sed 's/^/ /' "${WORK}/out" | ||
| FAILED=1 | ||
| fi | ||
| } | ||
|
|
||
| assert_fail() { | ||
| local desc="$1"; shift | ||
| if "$@"; then | ||
| echo "FAIL: ${desc} — expected the check to fail" | ||
| sed 's/^/ /' "${WORK}/out" | ||
| FAILED=1 | ||
| else | ||
| echo "PASS: ${desc}" | ||
| fi | ||
| } | ||
|
|
||
| assert_output_contains() { | ||
| local desc="$1" needle="$2" | ||
| if grep -qF -- "${needle}" "${WORK}/out"; then | ||
| echo "PASS: ${desc}" | ||
| else | ||
| echo "FAIL: ${desc} — output does not mention '${needle}'" | ||
| sed 's/^/ /' "${WORK}/out" | ||
| FAILED=1 | ||
| fi | ||
| } | ||
|
|
||
| # --- a clean release build --- | ||
|
|
||
| assert_pass "a build with neither flag is accepted" \ | ||
| run_checker nm-clean strings-clean | ||
|
|
||
| # --- -PenableFaultInjection markers --- | ||
|
|
||
| assert_fail "the faultinj:: namespace fails the check" \ | ||
| run_checker nm-faultinj strings-clean | ||
| assert_output_contains "the failure names the faultinj:: namespace" "faultinj:: namespace" | ||
| assert_output_contains "the failure names the offending flag" "-PenableFaultInjection" | ||
|
|
||
| assert_fail "the faults_injected counter string fails the check" \ | ||
| run_checker nm-clean strings-faultinj | ||
| assert_output_contains "the failure names the faults_injected counter" "faults_injected" | ||
|
|
||
| # --- -PenableSamplerPerf markers --- | ||
|
|
||
| assert_fail "the SamplerPerfProbe class fails the check" \ | ||
| run_checker nm-samplerperf strings-clean | ||
| assert_output_contains "the failure names the SamplerPerfProbe class" "SamplerPerfProbe" | ||
| assert_output_contains "the failure names the offending flag" "-PenableSamplerPerf" | ||
|
|
||
| assert_fail "the sampler_ticks./sampler_count. counter strings fail the check" \ | ||
| run_checker nm-clean strings-samplerperf | ||
| assert_output_contains "the failure names sampler_ticks" "sampler_ticks" | ||
| assert_output_contains "the failure names sampler_count" "sampler_count" | ||
|
|
||
| # --- symbol-table integrity --- | ||
|
|
||
| # An empty symbol table means the file could not be meaningfully read. | ||
| # Reporting a pass there would make the guard silently inoperative. | ||
| assert_fail "an empty symbol table is an error, not a pass" \ | ||
| run_checker nm-empty strings-clean | ||
| assert_output_contains "the empty-symtab error explains itself" \ | ||
| "found no symbols" | ||
|
|
||
| assert_fail "an nm that cannot read the file is rejected" \ | ||
| run_nm_unreadable | ||
| assert_output_contains "the unreadable-artifact error names the cause" "failed on" | ||
|
|
||
| # --- argument handling --- | ||
|
|
||
| assert_fail "a missing shared object is rejected" \ | ||
| run_raw "${WORK}/does-not-exist.so" | ||
| assert_output_contains "the missing-file error names the path" "no such file" | ||
|
|
||
| assert_exit_code "a missing shared-object argument exits with the usage status" 2 \ | ||
| run_raw | ||
|
|
||
| exit "${FAILED}" |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.