CodeVetter · Repo Unpack · Risk map
redis/redis
The dominant risk class is memory-safety in hand-written C parsing code reachable pre-authentication: the RESP parser, the cluster bus packet decoder, and the RDB/DUMP loader all consume untrusted bytes. Authentication is layered and easy to misconfigure — protected-mode, ACL, an auto-generated cluster internal secret, and module-supplied auth callbacks. A second class is coupling risk: the propagation invariants in `call()`/`propagateNow()`, the ordering contract inside `beforeSleep()`, and the object-type registration checklist all fail silently or assert if a contributor misses a step. Finally, there is genuinely dead code — GCRA is compiled out and its test file is `if 0`'d — and a large `DEBUG` surface with 50+ subcommands guarded only by a config flag.
- The RESP parser is the pre-auth attack surface: `processMultibulkBuffer()` and `processInlineBuffer()` parse attacker-controlled bytes into `pendingCommand` structs before any AUTH check, and `readQueryFromClient()` performs manual pointer arithmetic (`(size_t)(c->bulklen+2)-(sdslen(c->querybuf)-c->qb_pos)`) to size big-argument reads.
- The cluster internal secret is auto-generated with `getRandomHexChars(server.cluster->internal_secret, CLUSTER_INTERNALSECRETLEN)` and converged across nodes by a lexicographic `memcmp(...) > 0` rule in the PING extension handler — meaning any node that can join the bus can influence the shared secret used to authenticate internal connections.
- Internal-secret password comparison correctly uses `time_independent_strcmp()`, but it is one of several parallel auth paths (`checkPasswordBasedAuth`, `ACLAuthenticateUser`, module auth callbacks) that must all stay consistent — a change to one does not automatically cover the others.
- `moduleLoad()` calls `dlopen(path, RTLD_NOW|RTLD_LOCAL)` on an operator-supplied path, giving any successfully loaded module full in-process code execution; the only guard is that `MODULE` carries `CMD_PROTECTED` and is gated by `server.enable_module_cmd`.
- `warnAboutInsecureConfig()` only logs — it never refuses to start. A server with `DefaultUser` NOPASS, `protected-mode no` and a `0.0.0.0`/`::` bind emits a WARNING and then serves traffic.
- `redis-server --test-memory <mb>` and `--check-system` run arbitrary memory-stressing code (`memtest()`, `syscheck()`) directly from `main()` before any config or privilege handling, and `--test-memory` with a non-numeric argument silently becomes `atoi(...) == 0`.
- `src/debug.c` dispatches roughly 52 `DEBUG` subcommands via a chain of `strcasecmp(c->argv[1]->ptr, ...)` comparisons; the entire surface is protected only by the `enable-debug-command` config and `allowProtectedAction()`, and several subcommands (JMAP, SEGFAULT, SET-ACTIVE-EXPIRE, QUICKLIST-PACKED-THRESHOLD) can corrupt or crash a live server.
- Adding a new object type is a documented multi-file checklist with no compiler enforcement — the `server.h` comment requires coordinated edits to `rdb.c` (save/load, possibly RDB_VERSION), `aof.c` (rewrite), `db.c` (`obj_type_name`, `copyCommand`), `debug.c` (`xorObjectDigest`, `serverLogObjectDebugInfo`) and `defrag.c` (`defragKey`). Missing one produces silent data or defrag corruption, not a build error.
- GCRA is dead code in the shipped build: `src/gcra.c` is entirely inside `#ifdef ENABLE_GCRA`, `OBJ_GCRA`/`OBJ_TYPE_MAX 9` are conditional in `server.h`, the guard appears in `rdb.h`, `redismodule.h` and `acl.c`, and `tests/unit/gcra.tcl` disables its whole body with `if 0 {`. Building with `-DENABLE_GCRA` would activate untested code and change `OBJ_TYPE_MAX`, which is an RDB-visible constant.
- `beforeSleep()` encodes at least four ordering invariants in comments rather than in code structure (TLS pending data before AOF flush; `clusterBeforeSleep()` before `blockedBeforeSleep()`; `blockedBeforeSleep()` before `flushAppendOnlyFile`; AOF flush before `handleClientsWithPendingWrites`). Reordering any of them breaks `appendfsync=always` durability guarantees without failing a test deterministically.
- `propagateNow()` contains two assertions that convert logic errors into crashes rather than data loss: `debugServerAssert(duration != PROP_DURATION_UNKNOWN)` and the replica-pause assert. Both are reachable from any new command that queues propagation incorrectly.
- The query-buffer pooling scheme (`thread_reusable_qb` / `thread_reusable_qb_used` / `CLIENT_IO_REUSABLE_QUERYBUFFER`) is shared mutable state whose correctness depends on nested-execution detection; the code comment notes the private-buffer fallback exists only for `processEventsWhileBlocked()` re-entry.
- `src/module.c` is 16,117 lines — the single largest blast-radius file. It owns command registration, `RM_Call`, command filters, keyspace hooks, data-type serialization and `dlopen`, so nearly every subsystem change can break the stable module ABI in `src/redismodule.h`.
- Atomic slot migration carries its own trim-correctness risk: `ASM_DEBUG_TRIM_DEFAULT` auto-selects between background BIO-thread deletion and main-thread active deletion 'based on module subscriptions and client tracking', so keyspace notifications fire in one mode and not the other.
- `make` recipes shell out to scripts that perform network operations without review gates: `scripts/modules-update.sh` and `modules/common.mk` clone from GitHub URLs in `modules.yaml`, and `common.mk` resolves ref kind by probing the remote with `git ls-remote` at recipe time — so a `branch:` pin silently follows a moving target.
- CI does not run the slow test tag on PRs (`./runtest --verbose --tags -slow`), and the heavy `daily.yml` matrix only triggers on PRs to release branches matching `'[0-9].[0-9]'` — so changes merged to `unstable` get comparatively thin pre-merge validation.
- Sentinel mode initializes before config parsing and before the RDB/AOF check dispatch, and `sentinel.c` is 5,000+ lines sharing `server.c` globals with the normal server path — a startup-order change in `main()` can break sentinel in ways the default `./runtest` (which excludes sentinel) will not catch. (inferred)