Skip to content

Reject multiple source keys in BITOP NOT - #2041

Merged
kevin-montrose merged 4 commits into
microsoft:mainfrom
hexonal:fix-bitop-not-single-source-key
Aug 12, 2026
Merged

Reject multiple source keys in BITOP NOT#2041
kevin-montrose merged 4 commits into
microsoft:mainfrom
hexonal:fix-bitop-not-single-source-key

Conversation

@hexonal

Copy link
Copy Markdown
Contributor

Symptom

BITOP NOT is a unary operation — it inverts a single source key. Garnet silently accepts more than one source key and produces an undefined result instead of the error Redis returns.

Debug build of GarnetServer at 4ba5ebf:

C: SET short ab          # length 2
C: SET long abcdefghij   # length 10
C: BITOP NOT dst short long
S: :10
C: GET dst
S: "\x9e\x9d\x00\x00\x00\x00\x00\x00\x00\x00"

The destination takes the length of the longest source (10), the first two bytes are NOT("ab"), and the rest are zero — a result that corresponds to no well-defined operation. Redis rejects this outright:

BITOP NOT dst short long -> ERR BITOP NOT must be called with a single source key.

Root cause

libs/server/Resp/Bitmap/BitmapCommands.cs, NetworkStringBitOperation. The method validates the argument count for the variadic operators — at least one source key (parseState.Count < 2), the two-source minimum for DIFF, and the 64-key limit — but never enforces that NOT has exactly one source key. Extra keys are passed through to StringBitOperation, which processes them as if NOT were variadic.

Garnet already carries the sibling check for DIFF (RESP_ERR_BITOP_DIFF_TWO_SOURCE_KEYS_REQUIRED); the unary check for NOT was simply missing.

Fix

Add the unary check next to the existing DIFF check. parseState is [destkey, src1, ...], so a valid NOT has Count == 2:

if (bitOp == BitmapOperation.NOT && parseState.Count > 2)
{
    return AbortWithErrorMessage(CmdStrings.RESP_ERR_BITOP_NOT_SINGLE_SOURCE_KEY);
}

with RESP_ERR_BITOP_NOT_SINGLE_SOURCE_KEY => "ERR BITOP NOT must be called with a single source key."u8 (byte-for-byte the Redis message). Count < 2 is already rejected as wrong-number-of-arguments above, so this only rejects the extra-source case. AND/OR/XOR/DIFF are untouched.

Tests

GarnetBitmapTests.BitmapBitOpNotRejectsMultipleSourceKeys:

  • BITOP NOT dst a (single source) → still works, destination length 2.
  • BITOP NOT dst a b and BITOP NOT dst a b aERR BITOP NOT must be called with a single source key.
  • destination is unchanged by the rejected calls.
  • BITOP AND/OR/XOR dst a b still accept multiple sources (length 10).

Fails on unpatched main (the multi-source NOT is accepted, no exception thrown); passes with the fix. Full GarnetBitmapTests (359) stays green.

Docs

website/docs/commands/analytics.md described BITOP NOT as operating "between multiple keys" — a copy-paste artifact from the variadic operators that contradicts its own unary syntax line (BITOP NOT destkey srckey). Corrected to describe the single-source operation this change now enforces.

Copilot AI lite review requested due to automatic review settings August 7, 2026 17:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aligns Garnet’s BITOP NOT behavior with Redis by rejecting calls that provide more than one source key, eliminating the current undefined/accidental behavior where extra source keys are silently accepted.

Changes:

  • Added argument-count validation so BITOP NOT errors when more than one source key is provided.
  • Introduced a Redis-matching error string constant for the unary NOT violation.
  • Added a regression test covering both the accepted unary form and rejected multi-source forms, and corrected docs to describe unary NOT.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
libs/server/Resp/Bitmap/BitmapCommands.cs Enforces unary semantics for BITOP NOT (parseState.Count > 2 aborts with a specific error).
libs/server/Resp/CmdStrings.cs Adds RESP_ERR_BITOP_NOT_SINGLE_SOURCE_KEY with the Redis-compatible message text.
test/standalone/Garnet.test.complexstring/GarnetBitmapTests.cs Adds regression test ensuring multi-source NOT is rejected and variadic ops remain unaffected.
website/docs/commands/analytics.md Fixes BITOP NOT description to reflect unary behavior.

@hexonal
hexonal (hexonal) force-pushed the fix-bitop-not-single-source-key branch 3 times, most recently from 42a7e6e to a603585 Compare August 8, 2026 04:50
BITOP NOT is unary, but Garnet silently accepted extra source keys and
produced an undefined result (the destination took the length of the
longest source with the remaining bytes left zero) instead of the error
Redis returns. NetworkStringBitOperation validated the argument count for
the variadic operators and for DIFF, but never enforced that NOT has
exactly one source key.

Reject NOT with more than one source key, matching Redis' message. Also
correct the command doc, which described NOT as operating 'between
multiple keys', contradicting its own unary syntax.
@hexonal
hexonal (hexonal) force-pushed the fix-bitop-not-single-source-key branch from a603585 to ade2c73 Compare August 10, 2026 01:46
@kevin-montrose kevin-montrose self-assigned this Aug 11, 2026
@kevin-montrose
kevin-montrose merged commit c504ce0 into microsoft:main Aug 12, 2026
330 of 333 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants