MOD-17358 Reject short-form CLUSTERSET when the shard has no cluster identity by gabsow · Pull Request #103 · RedisGears/LibMR

gabsow · 2026-07-05T10:48:49Z

What

SetMyId() reports failure when the shard has no cluster identity, instead of copying from a NULL pointer. 21 insertions, 5 deletions.

Why

timeseries.CLUSTERSET's short form carries no MYID, so LibMR takes this shard's id from the server:

const char* myId = RedisModule_GetMyClusterID();
...
memcpy(cluster->myId + zerosPadding, myId, myIdLen);   // myId == NULL -> SIGSEGV

RedisModule_GetMyClusterID() is documented nullable, and the command is deliberately registered without the internal flag (unlike CLUSTERSETFROMSHARD and HELLO) so RAMP can see it in COMMAND LIST. The result is that on a shard with no cluster identity, any client can kill the server with one command:

redis-server --cluster-enabled no --loadmodule redistimeseries.so
TIMESERIES.CLUSTERSET      # -> SIGSEGV on the timeseries-el thread

That is the whole case for this PR now. It reproduces on a plain standalone shard with no Enterprise involvement, so it is fixed here regardless of what the platform does or does not send.

The id can also be NULL transiently in cluster mode, because Enterprise's cluster plugin clears myself on every topology apply — that is MOD-17358, and the root cause is fixed in redislabsdev/clusterlib#16. This guard is not the fix for it; once clusterlib#16 ships, this path stops being reachable on Enterprise. It still covers standalone shards and any build that predates it.

Scope

Narrowed from what this branch originally proposed. The RED-202230 guards (skip port <= 0 / empty ip, soft-skip NULL slot ranges) are gone — those conditions no longer exist:

port: fixed in the Redis fork by "Fix getNodeDefaultClientPort call with cluster_plugin (#1468)", 25 Jun 2026, on rl_8.4 e25184b9f, rl_8.6 823878e0d, rl_8.8 905417650, and rl_unstable. RED-202230 is closed Won't Do because it was solved there rather than in the plugin.
slot ranges: RM_GetClusterNodeSlotRanges returns slotRangeArrayCreate(0), never NULL.
ip: RM_GetClusterNodeInfo redis_strlcpys it on every REDISMODULE_OK path.

Keeping them would also have been actively risky: on a build still carrying the old port stub, skipping every port <= 0 node would have made MR_BuildCluster reject every short-form CLUSTERSET on Enterprise, disabling the feature as a side effect of a crash fix.

Shape

The check is where the value is used. SetMyId() already loads the id into a local, so checking it there means there is no second read to race with. MR_BuildCluster and SetClusterDataShortForm already return NULL / REDISMODULE_ERR, so this feeds paths that exist rather than adding new ones, and FreeCluster() is NULL-tolerant, so the cleanup is two lines. The long form is unaffected: it carries MYID in argv, so its InitClusterData() call asserts rather than branching.

Rejecting is what lets the caller recover: DMC's send_cluster_info_with_short_fallback already retries with the long form on an error reply. In the MOD-17358 crash it fired 13 ms after the send and found the shard dead.

Verification

RTS 8.8.2 (LibMR pin a37b672) built for linux-arm64 in rockylinux:9 + gcc-toolset-13, against a redis that exports GetClusterNodeSlotRanges, standalone shard, bare TIMESERIES.CLUSTERSET:

	before	after
reply	connection closed	`ERRCLUSTER Failed to set cluster topology`
shard	SIGSEGV `(nil)`, thread `timeseries-el`	alive (`PONG`)
crash reports	2	0

The reproduced stack matches the three field crashes frame for frame, including event_base_loop+0x2ac. A TS-side flow test rides with the module re-pin.

galcohen-redislabs · 2026-07-05T11:02:25Z

        return REDISMODULE_ERR;
    }

+    // The short form derives this shard's own identity and its peers entirely


Short-form should not be sent to non-ASM bdbs.
The change will only cause the dmc to send twice (i.e., 1. short -> rejected, 2. long).
Instead, the logic in the dmc should include the check for standalone vs cluster-aware shards.

Agree — DMC sending CLUSTERSET to a single-shard / non-cluster (cluster_enabled:0) DB is a DMC bug; there is no cluster topology to set there, so that is the trigger and the right place to stop it. Filing it for the DMC team (related: RED-201070).

Keeping the LibMR guard regardless, as defense-in-depth: the module must return an error, never SIGSEGV, on input it does not expect (RED-207632). Once DMC stops sending it the guard is simply never hit, so it costs nothing — it just guarantees no shard can be crashed this way again (by a retry, another component, or a future path).

Following up on this now that the DMC side shipped: it was implemented, and the shards still crash.

redis_cluster_enabled_state flips to true in one place — SMUpdateBDB.verify_restart(), and only for a redis_cluster_enabled update (cnm/cnm/statemachine/update.py:173). So it is a per-BDB latch meaning "the toggle finished", which closes exactly the flag-toggle window it was written for. Nothing re-evaluates it afterwards, and two windows are left open — both visible in the support packages:

1. Shards that start after the latch. redis-107 was sent a short-form CLUSTERSET 143 ms after Redis is starting, and 133 ms after ClusterPlugin: Waiting for topology to intialize... — trigger="per-shard retry requested by worker" target=107, with the flag enabled throughout. It survived by about 3 ms, because the plugin happened to apply epoch 21 at +5 ms. A crash-restart, a failover, or a shard newly added by a reshard all land in that window with no identity.

2. Settled shards, on every topology epoch. ClusterState::update_direct() logs Updating topology to epoch N, then clear() sets self.myself = None, and only later update_myself() restores it. reset is hard-coded true for every topology message (clusterlib_module/src/main_thread.rs:512) and shards run cluster-topology-poll-interval 1, so the window recurs about once a second for the life of the cluster. The fatal send's trigger was "master slot ranges changed" — the same event that nulls the id, 4 ms earlier. The thing that makes the DMC send the short form is the thing that makes it unsafe.

That is why this cannot be gated from the DMC: it is a race after the send, not a state that can be tested before it. Check, TCP, parse on the main thread, queue to the LibMR event-loop thread, and only then is GetMyClusterID() read — the window reopens in every one of those gaps.

Your fallback does fire, by the way: send_cluster_info_with_short_fallback 13 ms after the send. It just found the shard dead. That is the whole value of this guard — it turns a SIGSEGV into the error reply your fallback already knows how to handle, rather than adding any new recovery path.

cnm/cnm/statemachine/update.py:149 puts it well: "The DMC has an optimization that will crash the shards if redis_cluster isn't enabled." Narrowing the input is the right call and I am not arguing against it — but the module should not die on the input either way.

Correction to point 1 above — I had that wrong. The boot window is not reachable: clusterlib_module/src/lib.rs:194 blocks module load in an unbounded loop until the first topology is applied, so redis opens its listeners only afterwards and no CLUSTERSET can arrive before the shard has an id. Scratch the "survived by ~3 ms".

The second path is narrower but worse than I described. clear() nulls myself unconditionally, but it is restored only if let Some(nu) = msg.my_node_update, and that is populated only when the snapshot lists this shard — as a master with shard.master set, or as a replica. A snapshot that omits it leaves the identity NULL until the next one, i.e. ~1s at cluster-topology-poll-interval 1, not microseconds. Fix filed at redislabsdev/clusterlib#16.

Point 2 (the identity being dropped on every topology epoch) stands unchanged, and is the same PR.

…r identity Short form takes myId from GetMyClusterID(), which is NULL when the shard has no cluster identity; the NULL was memcpy()'d in SetMyId -> SIGSEGV on the LibMR event-loop thread. Reject at entry (checking both the cluster flag and the id value) instead of crashing. The long form is unaffected (myId comes from argv).

…usable endpoints GetClusterNodeInfo may return a zero/empty ip:port (e.g. RE without the node-port backport). Skip nodes with port <= 0 / empty ip; replace the slots != NULL assert with a soft skip; and if no valid master shards remain, free the half-built cluster and return an error so the caller falls back to the long form. Matches RediSearch's handling.

gabsow · 2026-07-05T12:37:41Z

@galcohen-redislabs your earlier comment was on the standalone guard (1st commit, RED-207632). Whats your take on the port / endpoint part (2nd commit, RED-202230)?

It brings the short form to RediSearch-parity in the node loop:

skip nodes where GetClusterNodeInfo gives no usable endpoint — rc != OK || port <= 0 || ip[0] == '\0' (the RE getNodeDefaultClientPort() → 0 case);
replace RedisModule_Assert(slots != NULL) with a soft skip;
if no valid master shards remain, return an error so DMC falls back to the long form.

Old behavior was: build a node with port 0 (silent misconfig), assert-crash on NULL slots, or reply OK with an empty topology.

Does this part look right to you to keep in the module, or do you see the same "DMC should handle it" argument applying here too?

galcohen-redislabs · 2026-07-05T16:50:11Z

@galcohen-redislabs your earlier comment was on the standalone guard (1st commit, RED-207632). Whats your take on the port / endpoint part (2nd commit, RED-202230)?

It brings the short form to RediSearch-parity in the node loop:

skip nodes where GetClusterNodeInfo gives no usable endpoint — rc != OK || port <= 0 || ip[0] == '\0' (the RE getNodeDefaultClientPort() → 0 case);

replace RedisModule_Assert(slots != NULL) with a soft skip;

if no valid master shards remain, return an error so DMC falls back to the long form.

Old behavior was: build a node with port 0 (silent misconfig), assert-crash on NULL slots, or reply OK with an empty topology.

Does this part look right to you to keep in the module, or do you see the same "DMC should handle it" argument applying here too?

It is also not needed, since the short form should not be sent in such cases (and if it is sent then we need to find out why and fix the root cause and definitely not skip empty results from GetClusterNodeInfo and hide a potential bug).

master moved SetClusterDataShortForm's topology build into MR_BuildCluster (RedisGears#104/RedisGears#105/RedisGears#117/RedisGears#119), so both guarded hunks moved with it: - the identity guard stays in SetClusterDataShortForm, next to the sibling slot-ranges-API guard; - the endpoint and slot-range guards move into MR_BuildCluster's node loop. Dropped this branch's 'no valid shards' reject: master's coveredSlots != NUMBER_OF_SLOTS check already rejects an under-covered topology, including the empty one.

gabsow · 2026-07-30T19:05:14Z

Reviving this — the crash is still happening, and the DMC-side fix cannot cover it.

@galcohen-redislabs your point was implemented: DMC now gates the short form on redis_cluster_enabled_state == ENABLED (RED-207632 #41752 + RED-208331 #41761). Crashes since then: ASM nightly 7585 (21 Jul), 7612 (28 Jul, test_asm_big_keys_impact), and RE 8.8 e2e 7604 (test_rest_api_reshard_4-8) — now tracked as MOD-17358.

None of them is the standalone case, so DMC is right to send the short form there: the shards are cluster_enabled:1, cluster_state:ok, ASM on. The identity disappears inside the shard, after the command is already in flight:

clusterlib/clusterlib/src/cluster_state.rs update_direct() logs Updating topology to epoch N, then clear() sets self.myself = None, and only later update_myself() puts it back.
reset is hard-coded true for every topology message (clusterlib_module/src/main_thread.rs:512), and shard confs carry cluster-topology-poll-interval 1, so that window recurs roughly every second.
Inside it get_my_cluster_node() returns null_mut(), so RedisModule_GetMyClusterID() returns NULL while cluster mode is reported on — which is why the guard checks the id and not just REDISMODULE_CTX_FLAGS_CLUSTER.

LibMR builds the topology on the GIL-less event-loop thread, so this is a race after the send — a pre-send check in DMC cannot close it. The crash is memcpy(dst, NULL, 40) in SetMyId: pc = __memcpy_simd+0x90 (its first source load), thread timeseries-el, and the timeseries-8.8.so offsets +0x45714 / +0x46134 / +0x494d8 / event_base_loop+0x2ac / +0x49534 are identical across all three support packages.

Verified on RTS 8.8.2 with this branch, against a redis that exports GetClusterNodeSlotRanges, standalone shard, bare TIMESERIES.CLUSTERSET:

	before	after
reply	connection closed	`ERRCLUSTER Failed to set cluster topology`
shard	SIGSEGV `(nil)` on `timeseries-el`	alive

Merged master in rather than rebasing, so the existing review threads stay anchored. Master moved the topology build into MR_BuildCluster, so the endpoint and slot-range guards moved with it; I dropped this branch's "no valid shards" reject because master's coveredSlots != NUMBER_OF_SLOTS already covers it. @AvivDavid23 — comments trimmed. PTAL.

cursor

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 722ada4. Configure here.}

The guard was at the entry of SetClusterDataShortForm while SetMyId went on to call RedisModule_GetMyClusterID() again, so the checked value and the used value were two different reads. Between them sit GetClusterNodesList(), a ~128KB MR_CALLOC and the argv strdups -- microseconds, the same order as the window where the plugin has no id to give. The guard made the crash unlikely, not impossible, and left the memcpy itself unguarded. Read the id once in MR_BuildCluster, which is where the server-derived build starts, and thread it through InitClusterData into SetMyId. Nothing re-reads it, so there is no window left to lose. SetMyId asserts non-NULL to keep the contract local and visible. Dropped the REDISMODULE_CTX_FLAGS_CLUSTER half of the guard: a shard that is not in cluster mode already gets NULL from RedisModule_GetMyClusterID(), so the id check subsumes it. The long form is unaffected -- it carries MYID in argv and passes NULL down. Verified on RTS 8.8.2 against a redis exporting GetClusterNodeSlotRanges, standalone shard, bare TIMESERIES.CLUSTERSET: before, SIGSEGV (nil) on the timeseries-el thread; after, "ERRCLUSTER Failed to set cluster topology" and the shard stays up.

gabsow · 2026-08-02T06:12:20Z

Reworked the identity guard — it never touched the code with the bug. The check sat at the function entry while SetMyId went on to call RedisModule_GetMyClusterID() again, so the checked value and the copied value were two different reads, with GetClusterNodesList(), a ~128 KB MR_CALLOC and the argv strdups in between. That made the crash unlikely, not impossible, and left the memcpy itself unguarded.

Now the id is read once in MR_BuildCluster and threaded through InitClusterData into SetMyId, which asserts non-NULL. Also dropped the REDISMODULE_CTX_FLAGS_CLUSTER half of the check — a shard not in cluster mode already gets NULL from the id, so it was redundant. The long form is unchanged: MYID comes from argv, and it passes NULL down.

Root cause is filed separately at redislabsdev/clusterlib#16 (clusterlib drops myself on every topology reset, and restores it only when the snapshot carries our node). If that merges, this guard stops being reachable on RE — but it still covers standalone shards and every RE build that predates it, so I'd keep both.

Supersedes the previous commit's shape. That one moved the RedisModule_GetMyClusterID() call out to MR_BuildCluster and passed the value down as a parameter. Same size either way (31 insertions), but a worse interface: SetMyId took an argument that only means anything for the short form, and it needed a comment at the call site, a comment on the parameter and an assert to explain itself. Keep the call where it belongs -- in the function whose job is to establish our id -- check it at the point of use, and report failure. MR_BuildCluster and SetClusterDataShortForm already return NULL / REDISMODULE_ERR, so this feeds paths that exist rather than adding new ones, and FreeCluster() is NULL-tolerant, so the cleanup on the reject path is two lines. The long form cannot fail here -- it carries MYID in argv -- so its InitClusterData() call asserts rather than branching.

gabsow · 2026-08-02T07:30:02Z

Switched the guard to report failure from SetMyId rather than threading the id in — Tom's call, and he's right.

I had claimed the previous shape (read the id in MR_BuildCluster, pass it down) was the leaner option. It isn't: both come to 31 insertions against master. Since size was the only argument for it, and it gave SetMyId a parameter that means something only for the short form — needing a comment at the call site, a comment on the parameter and an assert to explain itself — the version here now keeps RedisModule_GetMyClusterID() in the function whose job is to establish our id, checks it at the point of use, and returns REDISMODULE_ERR.

MR_BuildCluster and SetClusterDataShortForm already return NULL / REDISMODULE_ERR, so this feeds paths that already exist instead of adding any. FreeCluster() is NULL-tolerant, so the cleanup on the reject path is two lines. The long form still cannot fail here (MYID comes from argv), so its InitClusterData() call asserts rather than branching.

Re-verified after the change, RTS 8.8.2 / linux-arm64 against a redis exporting GetClusterNodeSlotRanges, standalone shard, bare TIMESERIES.CLUSTERSET: before — SIGSEGV (nil) on timeseries-el, shard dead; after — ERRCLUSTER Failed to set cluster topology, Short-form CLUSTERSET rejected: shard has no cluster identity, shard alive, 0 crash reports.

Note the intermediate commits will squash away on merge, so the history churn here is cosmetic.

RED-202230 is closed as Won't Do, which I read as "condition accepted". It was not: the callback was implemented in the Redis fork instead, so the ticket itself was never actioned. "Fix getNodeDefaultClientPort call with cluster_plugin (#1468)", 25 Jun 2026, is on rl_8.4 (e25184b9f), rl_8.6 (823878e0d), rl_8.8 (905417650) and rl_unstable: int getNodeDefaultClientPort(clusterNode *n) { return clusterNodeClientPort(n, server.tls_cluster); } So port is no longer 0 on any shipping line, and the other two guards were already unreachable: RM_GetClusterNodeSlotRanges returns slotRangeArrayCreate(0) rather than NULL, and RM_GetClusterNodeInfo redis_strlcpy's the ip on every REDISMODULE_OK path. All three were dead code, and they were most of the diff. Worse, on a build where the port stub had still been in place, the port <= 0 skip would have dropped every node and made MR_BuildCluster reject every short-form CLUSTERSET on Enterprise -- disabling the feature as a side effect of a crash fix. What is left is the identity guard, which stands on its own: timeseries.CLUSTERSET is deliberately registered without the internal flag, so on a shard with no cluster identity any client can crash the server with one command.

gabsow · 2026-08-02T08:58:22Z

@galcohen-redislabs @AvivDavid23 — scope cut right down, please re-look. Net diff is now 21 insertions, 5 deletions in one function's worth of change, down from the four-guard version you first saw.

Dropped the whole RED-202230 half. Those conditions no longer exist:

port: fixed in the Redis fork by "Fix getNodeDefaultClientPort call with cluster_plugin (#1468)", 25 Jun, on rl_8.4 e25184b9f, rl_8.6 823878e0d, rl_8.8 905417650, rl_unstable. RED-202230 is closed Won't Do because it was solved there instead of in the plugin — I had misread that as the condition being accepted.
RM_GetClusterNodeSlotRanges returns slotRangeArrayCreate(0), never NULL.
RM_GetClusterNodeInfo redis_strlcpys the ip on every OK path.

Keeping them would also have been risky rather than merely redundant: had any build still carried the old port stub, skipping every port <= 0 node would have made MR_BuildCluster reject every short-form CLUSTERSET on Enterprise — disabling the feature as a side effect of a crash fix.

What is left is the identity guard, and @galcohen-redislabs your original objection does not apply to it. You were right that DMC should not send the short form to a non-ASM bdb, and DMC now gates on redis_cluster_enabled_state — I verified that gate is present and working in the crashing builds. This is not about DMC's input. timeseries.CLUSTERSET is registered without the internal flag on purpose, so RAMP sees it in COMMAND LIST, which means any client can crash a shard that has no cluster identity:

redis-server --cluster-enabled no --loadmodule redistimeseries.so
TIMESERIES.CLUSTERSET      -> SIGSEGV on the timeseries-el thread

No Enterprise, no DMC, no race. A plain command should not be able to kill the server.

The Enterprise-side crash (MOD-17358) is a different NULL path — the cluster plugin clears myself on every topology apply — and its root cause is fixed in redislabsdev/clusterlib#16, not here. Once that ships this guard is unreachable on Enterprise; it still covers standalone and any older build.

Verified before/after on RTS 8.8.2 / linux-arm64: SIGSEGV (nil) on timeseries-el before, ERRCLUSTER Failed to set cluster topology and shard alive after.

galcohen-redislabs · 2026-08-17T20:50:16Z

@gabsow We should close this PR, since the bug was in the cluster plugin (and was fixed here).

This was referenced Jul 5, 2026

MOD-15749 Short-form CLUSTERSET: reject when the cluster API has no usable node endpoints #101

Closed

MOD-15749 Short-form CLUSTERSET: reject on non-cluster (standalone) shard (RED-207632) #102

Closed

gabsow force-pushed the tom.gabsow/MOD-15749-short-form-hardening branch from 8764666 to 7743798 Compare July 5, 2026 10:51

gabsow changed the title ~~MOD-15749 Short-form CLUSTERSET: harden against standalone shards and unusable endpoints (RED-207632, RED-202230)~~ RED-207632 Short-form CLUSTERSET: harden against standalone shards + unusable endpoints (RED-202230, MOD-15749) Jul 5, 2026

galcohen-redislabs reviewed Jul 5, 2026

View reviewed changes

gabsow force-pushed the tom.gabsow/MOD-15749-short-form-hardening branch from 7743798 to bae993c Compare July 5, 2026 11:11

gabsow requested review from AvivDavid23 and TalBarYakar July 5, 2026 11:24

AvivDavid23 reviewed Jul 5, 2026

View reviewed changes

Comment thread src/cluster.c Outdated

gabsow requested a review from galcohen-redislabs July 5, 2026 11:49

gabsow added 2 commits July 5, 2026 14:53

gabsow force-pushed the tom.gabsow/MOD-15749-short-form-hardening branch from bae993c to 1df3e44 Compare July 5, 2026 11:54

gabsow requested a review from AvivDavid23 July 30, 2026 19:05

cursor Bot reviewed Jul 30, 2026

View reviewed changes

Comment thread src/cluster.c Outdated

gabsow changed the title ~~RED-207632 Short-form CLUSTERSET: harden against standalone shards + unusable endpoints (RED-202230, MOD-15749)~~ MOD-17358 Reject short-form CLUSTERSET when the shard has no cluster identity Aug 2, 2026

gabsow closed this Aug 18, 2026

Navigation Menu

Uh oh!

MOD-17358 Reject short-form CLUSTERSET when the shard has no cluster identity - #103

MOD-17358 Reject short-form CLUSTERSET when the shard has no cluster identity#103
gabsow wants to merge 6 commits into
RedisGears:masterfrom
gabsow:tom.gabsow/MOD-15749-short-form-hardening

gabsow commented Jul 5, 2026 •

edited

Loading

Uh oh!

galcohen-redislabs Jul 5, 2026

Uh oh!

gabsow Jul 5, 2026

Uh oh!

gabsow Jul 30, 2026

Uh oh!

gabsow Jul 30, 2026

Uh oh!

Uh oh!

gabsow commented Jul 5, 2026

Uh oh!

galcohen-redislabs commented Jul 5, 2026

Uh oh!

gabsow commented Jul 30, 2026

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

gabsow commented Aug 2, 2026

Uh oh!

gabsow commented Aug 2, 2026

Uh oh!

gabsow commented Aug 2, 2026

Uh oh!

galcohen-redislabs commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

gabsow commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Scope

Shape

Verification

Uh oh!

galcohen-redislabs Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

gabsow Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

gabsow Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

gabsow Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gabsow commented Jul 5, 2026

Uh oh!

galcohen-redislabs commented Jul 5, 2026

Uh oh!

gabsow commented Jul 30, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gabsow commented Aug 2, 2026

Uh oh!

gabsow commented Aug 2, 2026

Uh oh!

gabsow commented Aug 2, 2026

Uh oh!

galcohen-redislabs commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gabsow commented Jul 5, 2026 •

edited

Loading