Repository navigation
ci(rdc): run the no-GPU tests under ASan and under UBSan - #14049
Merged
Merged
Conversation
1 task done
github-actions
Bot
added
the
github actions
This was referenced Oct 22, 2026
fix(rdc): rdci options, output and exit statuses, and the names and policy values RDC accepts
#14058
Draft
A new job, rdc_sanitize, has two legs. One builds RDC with GCC's -fsanitize=address and the other with -fsanitize=undefined, and each runs the installed gtests that need no GPU (all but rdc_diag_kernel_tests, which has no install RPATH), the fake-GPU suite and tests/ci against its build. GCC links the sanitizer runtime as a shared library, so rdcd, rdci, the gtests, the fake amd-smi and Python, which loads RDC's libraries through ctypes and gets the runtime through LD_PRELOAD, share one runtime. Every process writes its reports to one directory. A later step prints them, lists the distinct errors in the job summary and fails the job if there are any, if a report shows up instead in output the tests keep (such as rdcd's logs), or if the directory is missing because no test ran. The legs report what the other jobs cannot see, such as a read past a stack array or an invalid enum value that does not crash. The sanitizers get separate builds because in a process with both of GCC's runtimes, UBSan's reports go to stderr instead of to log_path (both runtimes export __sanitizer_set_report_path, and ASan's copy wins). The tests capture that stderr, so with one combined build the log directory had one report while rdcd's captured output held 270 UBSan errors. Before the tests, a step checks that rdcd, rdci, librdc, librdc_bootstrap and the fake amd-smi load the leg's runtime, that librdc calls the leg's checks, and that a program with a known bug leaves a report at log_path, so a build without the sanitizer fails instead of passing quietly. ASan's leak detection is off: Python under the runtime does not free everything at exit (151 allocations for "import ctypes, json"), and rdcd and rdci inherit its options. verify_asan_link_order is off because the tests preload the fake amd-smi, which links the runtime, ahead of it into rdcd; with the check on, rdcd exits at once. The legs build the same gRPC 1.67.1 as rdc_build and cache the install; each leg's ccache has its own key. Both are restored and saved in separate steps, since actions/cache saves nothing when a job fails. The legs name gcc and g++, build amd-smi with E-SMI and without a sanitizer, and build RDC without the runtime, profiler and RVS modules, which need ROCm's GPU runtime; rdcd runs without them. Each gtest has a time limit and writes a JUnit report, and the JUnit results go to the job summary as in the other test jobs. The legs do not gate until the repo variable RDC_SANITIZE_GATING is 'true'. On develop with the fake-GPU suite, in a GPU-less Ubuntu 22.04 container rather than the job's image, both legs' test runs pass every test: the 8 gtest programs the job runs (43 cases), 106 fake-GPU tests (24 skipped), and tests/ci but for test_prometheus_help, which needs prometheus_client (the job installs it). The ASan leg reports nothing. The UBSan leg reports, in 89 processes: a reference bound to a null pointer when RdcTelemetryModule passes &v[0] of an empty vector; an invalid rdc_field_type_t in fetch_smi_field() and the cache, because RDC_HEALTH_EEPROM_CONFIG_VALID reports success without setting a type or value, which then come from RdcSmiLib's uninitialized values array; the link type and P2P flag that rdc_device_topology_get() reads after a failed amd-smi call; and an invalid rdc_field_t where rdci dmon, the client library and rdcd load the field ID 99999 that a tests/ci test passes on purpose. GCC rather than clang for now: in local builds, ROCm's clang 22 with -shared-libsan reported ODR violations in the gtests (each string literal registered twice), and with clang's static runtime every process that loads RDC's instrumented libraries without being an instrumented executable (Python, setpriv, llvm-symbolizer) needed the runtime preloaded. A clang build like TheRock's is a follow-up. Signed-off-by: Adam360x <Adam.pryor@amd.com>
joshnguy-amd
force-pushed
the
users/adam360x/rdc-ci-sanitizers
branch
from
November 4, 2026 21:54
db0b967 to
ec58d80
Compare
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
The UBSan leg of rdc_sanitize reported 13 distinct findings in 89 processes. Nine of them come from three bugs: - RdcTelemetryModule passed &unsupport_fields[0] to the callback. The vector is usually empty, and indexing an empty vector binds a reference to null (87 processes). Use data(), which is defined for an empty vector. - fetch_smi_field set a field's status but never its type, so a successful integer field reached the cache and the log with an arbitrary rdc_field_type_t when the caller's value was uninitialised. Default the type to INTEGER; the string fields already set their own. - RdcTopologyLinkImpl used the output parameters of a failed amd-smi call. A failed call may still write them: the fake amd-smi writes a poison pattern, which UBSan reports as an invalid amdsmi_link_type_t and bool. Reset each output to N/A on failure, which is what the code's own comment says a failed lookup reports. After this the leg reports 4 findings in 2 processes, all rdc_field_t values that are not valid field ids. Those are covered by #14102 and #14160.
The source changes of #14102 (22d79f4), without its tests, so that the UBSan leg added by this PR runs clean. A caller can pass any 32-bit value as an rdc_field_t, and loading a value the enum does not define is undefined behavior. UBSan reports it for field ids such as 99999 in rdci dmon, rdci fieldgroup and rdcd's CreateFieldGroup request. Read the number from the bytes first (rdc_field_id_number), check it against the field table (is_field_id_valid, get_field_id_from_number), and convert to rdc_field_t only when it is a field RDC defines. rdcd now compiles rdc_fields_supported.cc to do the same check. With this the UBSan leg reports no findings: it was 13 before the earlier fix commit on this branch and 4 after it.
rdc_sanitize (address) failed on lsttstrix in test_rdcd_survives_a_failing_call[amdsmi_get_gpu_event_notification-fail]: rdcd printed "RDC server successfully shut down.", then AddressSanitizer reported a SEGV in the dynamic loader, and the exit code was 1. On SIGTERM the signal thread calls ShutDown(), which shuts the gRPC server down and then deletes the services, destroying the handler and waiting for the metrics updater's threads. Shutting the gRPC server down makes Run() return in main, which printed the message and returned at once, so the process started to exit while the signal thread was still deleting. The thread was detached, so nothing waited for it. Do not detach the signal thread; when main returns after a shutdown signal, join it first. The thread ends after ShutDown(), so the join returns. If the server fails to start, the thread is still looping and main does not join it, as before. The race did not reproduce locally: the failing test passed 40 of 40 runs with every CPU busy, and both sanitizer legs pass with and without this change. The cause comes from the code and from the order of the messages in the failing log. This overlaps #14048, which reorders the same shutdown more broadly.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the sanitizer CI additions and accompanying runtime fixes.
No open issues 🎉
What changed in this PR
Adds separate AddressSanitizer and UndefinedBehaviorSanitizer CI jobs for RDC’s GPU-less tests, building on the fake amd-smi suite. The diff also includes runtime fixes for shutdown, field-ID validation, and telemetry handling.
Changes:
- Build and run GPU-less tests under each sanitizer, verify instrumentation, and publish reports with optional gating.
- Validate numeric field IDs before enum conversion and preserve raw IDs across client requests.
- Join the shutdown thread, reset failed topology outputs, initialize metric types, and avoid indexing an empty telemetry vector.
| File | Description |
|---|---|
projects/rdc/server/src/rdc_server_main.cc |
Join the signal thread before successful shutdown exit. |
projects/rdc/server/src/rdc_api_service.cc |
Validate incoming field IDs before enum conversion. |
projects/rdc/server/CMakeLists.txt |
Link field-validation helpers into the server. |
projects/rdc/rdci/src/RdciFieldGroupSubSystem.cc |
Validate numeric field IDs for field-group operations. |
projects/rdc/rdci/src/RdciDmonSubSystem.cc |
Validate numeric monitoring field IDs. |
projects/rdc/rdc_libs/rdc/src/RdcTopologyLinkImpl.cc |
Reset topology outputs after failed amd-smi calls. |
projects/rdc/rdc_libs/rdc/src/RdcTelemetryModule.cc |
Use data() instead of indexing a potentially empty vector. |
projects/rdc/rdc_libs/rdc/src/RdcMetricFetcherImpl.cc |
Initialize the metric value type to integer. |
projects/rdc/rdc_libs/rdc/src/RdcEmbeddedHandler.cc |
Validate caller-provided field IDs as numbers. |
projects/rdc/rdc_libs/rdc_client/src/RdcStandaloneHandler.cc |
Serialize raw numeric field IDs for server validation. |
projects/rdc/include/rdc_lib/rdc_common.h |
Add byte-copy extraction of numeric field IDs. |
projects/rdc/common/rdc_fields_supported.h |
Declare numeric field-ID validation and parsing helpers. |
projects/rdc/common/rdc_fields_supported.cc |
Implement numeric field-ID validation and parsing. |
.github/workflows/rdc-ci.yml |
Add sanitizer builds, runtime checks, test execution, and reporting. |
Review effort: Balanced
bill-shuzhou-liu
approved these changes
Nov 6, 2026
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.
Motivation
RDC's tests run without sanitizers, so a read past a buffer or an invalid enum value that does not crash goes unseen.
Technical Details
rdc_sanitize, has two legs: GCC-fsanitize=addressand-fsanitize=undefined, one build each. With both runtimes in one process, UBSan's reports go to stderr instead oflog_path.rdc_diag_kernel_tests, which has no install RPATH), the fake-GPU suite (test(rdc): run rdcd and rdci in CI against a fake amd-smi #14041) andtests/ciagainst its build. Every process writes its reports to one directory.rdcd's logs) instead, or if no test ran.log_path.rdcdandrdciinherit its options), and so isverify_asan_link_order(the tests preload the fake amd-smi, which links the runtime, intordcd).rdc_builddoes and cache it. They do not gate until the repo variableRDC_SANITIZE_GATINGistrue.CI-only; no CHANGELOG entry.
Issue Tracking
ISSUE ID : #14107
Part of a series of RDC fixes, which #14107 lists. No JIRA ticket.
Test Plan
Ran each leg's build and test steps on this branch in a GPU-less Ubuntu 22.04 container.
Test Result
Both legs pass every test: the 8 gtest programs the job runs (43 cases), 106 fake-GPU tests (24 skipped), and
tests/ci(92 passed, 6 skipped;test_prometheus_helpneedsprometheus_client, which the job installs).The ASan leg reports nothing. The UBSan leg reports, in 89 processes, findings that are on develop today:
RdcTelemetryModulepasses&v[0]of an empty vector (the telemetry fix in this series, fix(rdc): bound the fields and devices rdcd handles at once #14045, removes it),rdc_field_type_tinfetch_smi_field()and the cache, becauseRDC_HEALTH_EEPROM_CONFIG_VALIDreports success without setting a type or value,rdc_device_topology_get()reads after a failed amd-smi call,rdc_field_twhererdci dmon, the client library andrdcdload the field ID 99999 that atests/citest passes on purpose.Submission Checklist