Harden rDSN core, RPC, and tooling against crashes on recoverable errors - #272
Merged
Conversation
added 12 commits
July 18, 2026 08:56
RPC dispatch acquires a running_count reference while the handler is protected by the dispatcher lock. Normal queued execution consumed that reference in rpc_handler_info::run, but three exits bypassed it: hosted-app inline dispatch called the C handler directly, timeout-based request dropping skipped run, and pre-enqueue fault injection destroyed the request task before execution. Those paths permanently inflated running_count and prevented dynamically allocated handlers from being deleted after unregistration. Repeated handler lifecycles therefore leaked handler allocations, while layer-2 handlers accumulated stale counts. Balance the inline reference after the callback. Treat rpc_request_task's existing handler pointer as its ownership marker, clear it after run consumes the reference, and release any still-owned reference when an expired request is dropped or a task is destroyed before execution. This preserves callback, scheduling, registration, and object-layout semantics. Add focused regressions for inline execution, expired requests, and discarded request tasks.
dsn_rpc_register_handler returns bool so callers can recover from a duplicate task-code or handler-name registration. The dispatcher nevertheless executed an always-on dassert in its conflict branch, terminating dsn.svchost with SIGABRT before the existing false return could run. Replace the fatal assertion with an error log so the rejected registration follows the established failure path. The registered handler remains untouched and the C and C++ wrappers can clean up the rejected handler context normally. Add a focused regression that registers the same RPC task code twice, verifies the second call returns false, and confirms unregistering still returns the first handler's context.
Symptom:
In rpc_engine::call_uri(), the exponential-backoff delay for retrying a
URI-addressed (partitioned) RPC was computed as:
uint64_t gap = 8 << req2->send_retry_count;
The literal 8 is a signed 32-bit int, so this is a 32-bit signed shift.
send_retry_count (int, initialized to 0, only ever incremented) grows
unbounded while a long-timeout RPC keeps retrying a persistently failing
partition. Once the exponent reaches ~28 the result overflows INT_MAX
(signed-overflow UB); at >=32 it is an oversized-shift UB. The widening to
uint64_t happens only after the 32-bit shift, so it does not help.
Root cause:
32-bit signed base (8) combined with an unbounded shift exponent
(send_retry_count). Retries fire ~1/s (gap capped at 1000ms), so ~23s of a
blackholed/unreachable partition on a long-timeout call reaches count 28,
the start of signed-overflow UB; a minutes-long client timeout reaches 32+
(oversized shift). No attacker input is required.
Fix:
Clamp the shift exponent to 10 and use a 64-bit unsigned base:
int retry_shift = req2->send_retry_count;
if (retry_shift > 10) { retry_shift = 10; }
uint64_t gap = 8ull << retry_shift;
This is byte-identical to the original intent for every send_retry_count:
counts 0..6 give 8..512 (uncapped); counts >=7 give >=1024, which the
pre-existing "if (gap > 1000) gap = 1000;" cap already clamps to 1000, so
clamping the exponent at 10 (8ull<<10 = 8192, still >1000) leaves the final
gap unchanged for every count. send_retry_count is always >=0, so there is
no negative-shift concern.
Validation:
- Standalone -fsanitize=undefined demo: the old form trips UBSan
("shift exponent too large") at exponent >=28; the new form is clean
across counts 0..63 and yields identical gap values across the
well-defined range.
- Incremental "make dsn.core -j 48" on Ubuntu 16.04 compiles cleanly.
In run() (src/core/src/main.cpp) each ';'-separated app_list entry is split on
'@' into name@index parts via split_args(). split_args drops empty/whitespace-
only tokens, so a malformed -app_list entry consisting only of '@' separators
(e.g. -app_list "@", "@@", or a trailing "realapp;@") produces an empty
argskvs list. The code then called argskvs.front() unguarded; front() on an
empty std::list dereferences the end sentinel (undefined behavior), and the
following ("apps." + front()) reads garbage as a safe_string, causing a
startup crash (SIGSEGV / stack-buffer-overflow under ASan). The existing
argskvs.size() < 2 guard ran only after the bad front().
Skip an entry that produced no tokens before dereferencing front(): such an
entry names no app and cannot match any config section, so the behavior for
every valid "app" or "app@index" entry is unchanged.
Advance the rDSN.tools.hpc submodule to include the HPC-provider hardening fixes (886c8c9..2025ee8): correct libaio result decode in complete_aio; convert always-on dassert aborts on recoverable OS/IO errors to graceful derror/retry in the aio provider destructor and completion callback, the network provider socket-creation paths, the io_looper eventfd/kevent wakeup and fcntl bind, and the hpc_logger directory scan; and fix the shared io_looper _events[] cross-thread data race by making the ready-event buffer thread-local per worker.
Convert ten genuine defects (SIGFPE / process abort / out-of-bounds read / divide-by-zero) into graceful error returns, clamps, or early exits. Each site gains a diagnostic and a recoverable path instead of terminating the process. Core runtime: - task_spec::init(): reject a thread-pool worker_count < 1. A partitioned pool with worker_count == 0 sized its queue/worker vectors empty, so enqueue() and shared_same_worker_with_current_task() did hash() % 0 -> SIGFPE; a non-partitioned pool silently hung with no workers. Validate at the init() choke point and fail startup cleanly. Core-adjacent: - service_engine::start_node(): a network port collision hit dassert(false, "port confliction") and aborted the node. Return nullptr through the existing null-checked path instead. - env_provider::random64(min, max): an inverted range (min > max) aborted on dassert(min <= max). Warn and return min instead. Tools/plugins (config-gated): - simple_perf_counter_v2_atomic / _fast: a computation interval < 1 made rand() % interval a divide-by-zero. Clamp to 1, matching the base counter. - tools.emulator network.sim start(): an empty hostname or an unsupported channel aborted via dassert. Return ERR_NETWORK_INIT_FAILED / ERR_NOT_IMPLEMENTED. Sample app (apps.skv): - simple_kv COPY-checkpoint apply_checkpoint(): guard file_state_count <= 0 before dereferencing files[0] (out-of-bounds read); convert the decree precondition dassert to an ERR_CHECKPOINT_FAILED return; on shutdown, a failed remove_path no longer aborts. Benchmark harness (dev/cpp): - perf_test_helper: clamp key_space_size < 1 to 1 so the client's random % key_space_size cannot divide by zero.
Convert always-on dassert/dassert(false) on recoverable OS/config/ untrusted-input errors into graceful derror + early-return/clamp so a bad config value, a missing provider name, or an OS call failure logs and degrades instead of aborting the process. Genuine internal invariants stay fail-fast. The dominant sub-class: factory_store::create returns nullptr (it does not abort) for a missing/wrong-type provider name, so any config-supplied provider name followed by an unchecked deref is a config-reachable null-deref; those sites now null-check. - network.cpp get_local_ipv4(): gethostname() failure dassert(false) -> derror + return 0 (OS-reachable). - perf_test_helper.cpp (ctor + load_suite_config): two read-config dassert(false) -> derror + continue/return (dev/tool code). - simple_logger.cpp: invalid stderr_start_level config yielded LOG_LEVEL_INVALID -> dassert; fall back to LOG_LEVEL_WARNING + stderr. - task_spec.cpp: rpc_request_delays_milliseconds size guard protected an OOB write into fixed int _delay[6]; on bad size derror + return false instead of dassert. - rpc_engine.cpp create_network(): null-check the create() result and the aspect-loop result -> derror + return nullptr. - task_engine.cpp: null-check queue and worker factory results after the aspect loops -> derror + dsn_exit(1). - main.cpp run(): toollet-not-found dassert -> derror + return false. - service_engine.cpp init_io_engine(): null aio provider guard -> derror + return ERR_SERVICE_NOT_FOUND. - service_engine.cpp/.h init_after_toollets(): void -> error_code, guard null env provider before assigning tls_dsn.env; main.cpp caller checks the returned code (::dsn::ERR_OK) and fails gracefully.
Point the dist.service submodule at 5eeeda8, which converts recoverable dassert()/dassert(false) aborts on OS/config/untrusted-input errors into graceful handling, fixes out-of-bounds and readlink stack/uninitialized-read hazards in the CLI tools, and makes the app_daemon fork/exec child async-signal-safe (all allocation, env/argv assembly and logging moved before fork(); the child issues only async-signal-safe syscalls with an explicit execve envp, so no setenv). Submodule commit (linmajia/rDSN.dist.service @ fix): meta_server_lib/server_state.cpp, meta_server_lib/greedy_load_balancer.cpp, replication_lib/replica_init.cpp, app_daemon/daemon.server.cpp, tools/repli/repli.main.cpp, tools/ddl_client/main.cpp
Parent fixes: - file_utils.cpp get_process_image_path(): on Linux, readlink() does not null-terminate and can return up to TLS_PATH_BUFFER_SIZE, so writing the NUL at tls_path_buffer[err] was a one-byte out-of-bounds write for paths whose length reaches the buffer size. Clamp to the last valid index (mirrors the FreeBSD/Apple branches). On Windows, close the OpenProcess handle on every exit path -- it was leaked when QueryFullProcessImageNameA failed; the GetCurrentProcess pseudo-handle (pid == -1) is correctly left un-closed. - group_address.h possible_leader(): it lazily assigns _leader_index, which is a data race under the shared read lock (the sibling writers leader_forward() and set_leader() take the write lock). Use double-checked locking: keep the hot "leader already elected" path on the read lock, and fall back to the write lock only for the one-time election, re-checking _members/_leader_index under it. rw_lock_nr is non-recursive, so the read lock is released before the write lock is taken (no lock upgrade / self-deadlock). Submodule bumps: - rDSN.dist.service: harden meta service startup against null provider factories (meta_state_service, server_load_balancer, distributed_lock_service). - rDSN.dist.deployment: harden the docker/kubernetes deployment schedulers against dassert on recoverable external-command / config / I-O failures.
asio_net_provider: the RPC channel is configurable, so a channel other than TCP/UDP is a recoverable misconfiguration. asio_network_provider::start and asio_udp_provider::start now log the error and return ERR_NOT_IMPLEMENTED through their existing error_code path instead of a dassert that would coredump the host at startup. Submodule pointers advanced to their Round-86 fix commits: - rDSN.dist.deployment: cluster-config loading hardening - rDSN.tools.hpc: unsupported network-channel graceful reject - rDSN.tools.log.monitor: invalid log-monitor master-address tolerance
All four submodule hardening PRs were merged into their master branches; advance the gitlinks from the now-deleted fix branches to the merged master tips: - rDSN.dist.service: 411a379 -> b34bb90 (PR microsoft#39) - rDSN.dist.deployment: b428de3 -> 7a9dc73 (PR microsoft#9) - rDSN.tools.hpc: a6c708f -> 98d8699 (PR microsoft#15) - rDSN.tools.log.monitor: de65e5c -> e92b56b (PR #6)
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
1 participant
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.
Summary
This PR lands an accumulated batch of robustness-hardening fixes found through
fault-injection (libfiu /
fiu-run) testing on Ubuntu. The unifying principle:convert always-on
dassert/abort on recoverable external conditions (OS errors,configuration values, untrusted network/disk input) into graceful handling
(
derror/dwarn+ early-return / clamp / null-check), and fix a set of latentmemory, undefined-behavior, and leak bugs. The healthy/hot path is unchanged — the
process now degrades or fails cleanly instead of coredumping the whole host.
It also advances all four plugin submodules to their merged
mastertips (theirindividual hardening PRs are already merged).
Core runtime & task system
worker_count(task_spec.cpp): rejectworker_count < 1up front.A partitioned pool with 0 workers divides by
_queues.size()/_workers.size() == 0(SIGFPE) in
enqueue()/shared_same_worker_with_current_task(); a non-partitionedpool with 0 workers silently hangs (tasks enqueued, never processed).
task_engine.cpp): a null result fromfactory_store::create()for a queue/worker now logs a clear configuration error anddsn_exit(1)instead of dereferencing null.env_provider.cpp):random64(min, max)withmin > maxnow returnsminwith adwarninstead of asserting — and, with asserts compiled out, avoidsfeeding an invalid range to
uniform_int_distribution(UB).RPC layer
rpc_engine.cpp,tool-api/task.h,task.cpp): fixref-count leaks of RPC handler tasks; covered by new
src/core/src/rpc.test.cpp.rpc_engine.cpp): registering an already-registered RPChandler reports an error instead of aborting; covered by new
service_api_c.test.cpp.rpc_engine.cpp): fix undefined behavior in the retrybackoff left-shift (shift count could reach/exceed the operand width).
rpc_engine.cpp): a null network / network-aspectprovider from the factory now fails gracefully (
derror+return nullptr) instead ofa null dereference.
Startup, config & service engine
main.cpp,service_engine.cpp/.h): an unknown toollet anda failed
init_after_toollets()now return a clean error;init_after_toollets()waschanged to propagate an
error_code. Null AIO/env providers from the factory returnERR_SERVICE_NOT_FOUNDinstead of a null dereference.service_engine.cpp): a configured port collision between two appslogs an error and fails node creation (callers already handle a
nullptrreturn)instead of
dassert(false).app_listentry (main.cpp): skip empty entries to avoidfront()on anempty container.
gethostname(network.cpp): failure returns 0 with aderrorinstead ofproceeding on garbage.
Networking, logging & plugin tooling
tools.common/asio_net_provider.cpp): the TCP/UDP-only channelcheck in
asio_network_provider::start/asio_udp_provider::startreturnsERR_NOT_IMPLEMENTEDfor a misconfigured channel instead of aborting.tools.emulator/network.sim.cpp): equivalent graceful channelhandling.
tools.common/simple_perf_counter_v2_fast.cpp,simple_perf_counter_v2_atomic.cpp): clamp a configured interval of 0 to 1 to avoidrand() % 0(SIGFPE).tools.common/simple_logger.cpp): an invalidstderr_start_levelconfig value falls back to a default instead of aborting logger initialization.
dev/cpp/perf_test_helper.cpp): malformed benchmark configsections are reported and skipped (defaults used) instead of aborting the process.
Storage / app (simple_kv)
apps.skv/simple_kv.server.impl.cpp): guardfile_state_count <= 0(the COPY branch dereferencesfiles[0]) and a non-increasingcheckpoint decree, returning
ERR_CHECKPOINT_FAILEDinstead of an OOB/null-deref +abort; a shutdown directory-cleanup failure now logs instead of asserting.
Process/path utilities & concurrency
dev/cpp/file_utils.cpp):get_process_image_path()clamps thereadlink()length before NUL-terminating (a 1-byte OOB write when the path exactlyfilled the buffer) and closes the Windows process handle on all return paths.
core/src/group_address.h):possible_leader()no longerwrites
_leader_indexunder a read lock — the one-time lazy election upgrades to a writelock (double-checked locking) to remove a data race.
Submodule updates (each already merged to its
master)b34bb90— meta-service null-provider-factory guards; abort/OOB/unsafe-fork hardening7a9dc73— deployment scheduler config-loading hardening98d8699— HPC provider robustness; critical-log to stderre92b56b— tolerate invalid log-monitor master addressTesting & verification
src/core/src/rpc.test.cpp(handler ref-leak),src/core/src/service_api_c.test.cpp(duplicate registration + config key-count contract).(
rm -fr builder && CC=gcc CXX=g++ ./run.sh build --build_plugins -j 48) →Build succeed, 0 warnings/errors on any touched file.fiu-runused to exercise the recoverable error paths.Full multi-OS test-suite runs are performed separately on the maintainer's dev machines.