Fix execGetKeys() dropping the first 256 keys past the static buffer

[madolson]

COMMAND GETKEYS EXEC with more than 256 conditions returns the wrong keys and can crash the server. execGetKeys() leaves result->numkeys at 0 until after its loop, but getKeysPrepareResult() copies exactly result->numkeys entries when it grows off the 256-entry static buffer, so the first 256 keyReference entries are discarded and replaced with uninitialised heap. On a freshly started server that heap reads as zero, pos is 0, and the reply is 256 copies of EXEC; after any real workload pos is arbitrary and the reply builder dereferences it. This publishes the count as each entry is written.

Details

Problem

execGetKeys() appends one key per condition at src/multi.c:246-249, growing the result with getKeysPrepareResult(result, numkeys + 1), and only publishes the total afterwards.

getKeysPrepareResult() at src/db.c:2368-2376 branches on whether it is still using the caller's static buffer. When it moves off keysbuf it allocates and copies result->numkeys entries:

} else {
    /* We are using a static buffer, copy its contents */
    result->keys = zmalloc(numkeys * sizeof(keyReference));
    if (result->numkeys) memcpy(result->keys, result->keysbuf, result->numkeys * sizeof(keyReference));
}

result->numkeys is still 0 at that point, so nothing is copied and keys[0..255] are whatever the fresh block holds. MAX_KEYS_BUFFER is 256 (src/server.h:2535) and initGetKeysResult() sets result->size to it (src/server.h:2556).

Every other incremental caller keeps the field in step, for example getKeysUsingKeySpecs() at src/db.c:2474, which is why nothing else trips this.

Introduced by #4019, which added the condition arguments and this getkeys proc.

Reachability

Wrong keys, on a server started moments earlier:

COMMAND GETKEYS EXEC IFEQ k0 v0 ... (300 conditions)
  -> first 256 entries all "EXEC" (argv[0]), last 44 correct k256..k299
  -> at exactly 256 conditions, all correct

Crash, after any churn (40 rounds of a Lua loop setting and deleting 200 keys is enough):

valkey 255.255.255 crashed by signal: 11, si_code: 1
Accessing address: 0x7f2e18d0e010
EIP: getKeysSubcommandImpl+0x1b3

COMMAND is not privilege-gated by default, and cluster-aware clients and proxies call COMMAND GETKEYS to route commands they do not parse themselves.

Plain MULTI/EXEC does not reach it. With the same dirtied heap and 300 conditions, standalone replies *-1 and cluster-enabled replies -CROSSSLOT, neither crashing. A user limited to ~k* gets NOPERM with the denied key at condition 0, at condition 260, and at 256 conditions, so there is no ACL bypass at pos 0; whether a non-zero pos can fail open is heap dependent and I could not construct it.

Alternative considered

Making getKeysPrepareResult() copy result->size entries, or the requested count, instead of result->numkeys would protect any future caller written this way. It loses because copying result->numkeys is the contract every other caller already honours, so changing the helper changes the contract for all of them to accommodate the one that broke it.

Testing

The added test fails without the src/multi.c change:

[err]: EXEC conditions past the static key buffer report the real keys in tests/unit/multi.tcl
Expected 'key:0' to be equal to 'EXEC'

This was generated by AI but verified, with love, by a human.