From 52dd34261aec98e30c8b5af2b09050c50081bba3 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Tue, 29 Sep 2026 18:38:44 +0000 Subject: [PATCH] Preventing fault-injection and sampler perf counting in production build --- .github/workflows/ci.yml | 7 +- .gitlab/scripts/build.sh | 4 + .gitlab/scripts/check-release-flags.sh | 110 ++++++++++ .../scripts/tests/check_release_flags_test.sh | 200 ++++++++++++++++++ 4 files changed, 320 insertions(+), 1 deletion(-) create mode 100755 .gitlab/scripts/check-release-flags.sh create mode 100755 .gitlab/scripts/tests/check_release_flags_test.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2487939baa..2276bcd2a6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,12 +60,17 @@ jobs: bash -n \ .gitlab/scripts/build.sh \ .gitlab/scripts/check-abi-floor.sh \ + .gitlab/scripts/check-release-flags.sh \ .gitlab/scripts/tests/check_abi_floor_test.sh \ + .gitlab/scripts/tests/check_release_flags_test.sh \ .gitlab/scripts/tests/includes_test.sh shellcheck \ .gitlab/scripts/check-abi-floor.sh \ - .gitlab/scripts/tests/check_abi_floor_test.sh + .gitlab/scripts/check-release-flags.sh \ + .gitlab/scripts/tests/check_abi_floor_test.sh \ + .gitlab/scripts/tests/check_release_flags_test.sh bash .gitlab/scripts/tests/check_abi_floor_test.sh + bash .gitlab/scripts/tests/check_release_flags_test.sh bash .gitlab/scripts/tests/includes_test.sh # Fails when a quarantine entry is malformed, ticketless, or past its diff --git a/.gitlab/scripts/build.sh b/.gitlab/scripts/build.sh index 94bd6e8b74..4e70c37d2b 100755 --- a/.gitlab/scripts/build.sh +++ b/.gitlab/scripts/build.sh @@ -61,6 +61,10 @@ set -x mkdir -p "${REPO_ROOT}/libs" cp -r "${REPO_ROOT}/ddprof-lib/build/native/release/META-INF/native-libs/"* "${REPO_ROOT}/libs/" +# Refuse to ship a build made with the opt-in debug/perf flags: applies to +# every target, unlike the ABI floor check below. +"${HERE}/check-release-flags.sh" "${REPO_ROOT}/libs/${TARGET}/libjavaProfiler.so" + # Assert what the artifact requires from the host that will load it. musl # targets carry no glibc symbol versions, so the check does not apply to them. case "${TARGET}" in diff --git a/.gitlab/scripts/check-release-flags.sh b/.gitlab/scripts/check-release-flags.sh new file mode 100755 index 0000000000..8c9b4fb0a0 --- /dev/null +++ b/.gitlab/scripts/check-release-flags.sh @@ -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 +# 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 +# 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") " >&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" + +# "::::". 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)' + '_ZN16SamplerPerfProbe::-PenableSamplerPerf::the SamplerPerfProbe class (samplerPerf.h)' +) + +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}" +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}" + +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 + 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}" diff --git a/.gitlab/scripts/tests/check_release_flags_test.sh b/.gitlab/scripts/tests/check_release_flags_test.sh new file mode 100755 index 0000000000..cb59e3e1a8 --- /dev/null +++ b/.gitlab/scripts/tests/check_release_flags_test.sh @@ -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}"