From c3ddd1aef5c7b142b44829d308adbacba98c1532 Mon Sep 17 00:00:00 2001 From: hexonal Date: Thu, 23 Jul 2026 00:10:53 -0400 Subject: [PATCH 1/2] Fix one-byte out-of-bounds read in BITPOS with explicit end offset BitPosDriver clamped an out-of-range explicit end offset to inputLen (BYTE mode) / bitLen (BIT mode) instead of inputLen-1 / bitLen-1, i.e. one index past the last valid position. BitPosByteSearch and BitPosBitSearch iterate while currentOffset <= endOffset over an unsafe byte*, so BITPOS key 0 0 against a value with no matching bit reads one byte past the end of the value's backing buffer. BitCountDriver's BYTE-mode path already clamps to valLen-1 (fixed in #1138), and real Redis clamps end to totlen-1 in bitposCommand for both BYTE and BIT indexing; this brings BitPosDriver in line with both. Both offsetType branches had the same off-by-one, so both are fixed here. BitPosByteSearch/BitPosBitSearch are private and only reachable through BitPosDriver, so the single clamp fix closes the read for every caller; no additional bounds check inside the search loops is needed. Adds two regression tests that call BitmapManager.BitPosDriver directly against an unmanaged buffer with a canary byte placed one past the declared value length, so the overread is deterministically observable instead of depending on incidental adjacent-heap content. Confirmed both tests fail against the pre-fix clamp (returning a position inside the canary byte) and pass with the fix. --- .../server/Resp/Bitmap/BitmapManagerBitPos.cs | 4 +- .../GarnetBitmapTests.cs | 71 +++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/libs/server/Resp/Bitmap/BitmapManagerBitPos.cs b/libs/server/Resp/Bitmap/BitmapManagerBitPos.cs index ff8f3237598..c0dea36c0f4 100644 --- a/libs/server/Resp/Bitmap/BitmapManagerBitPos.cs +++ b/libs/server/Resp/Bitmap/BitmapManagerBitPos.cs @@ -31,7 +31,7 @@ public static long BitPosDriver(byte* input, int inputLen, long startOffset, lon if (startOffset > endOffset) // If start offset beyond endOffset return 0 return -1; - endOffset = endOffset >= inputLen ? inputLen : endOffset; + endOffset = endOffset >= inputLen ? inputLen - 1 : endOffset; // BYTE search return BitPosByteSearch(input, inputLen, startOffset, endOffset, searchFor); } @@ -50,7 +50,7 @@ public static long BitPosDriver(byte* input, int inputLen, long startOffset, lon if (startByteIndex > endByteIndex) // If start offset beyond endOffset return 0 return -1; - endOffset = endByteIndex >= inputLen ? bitLen : endOffset; + endOffset = endByteIndex >= inputLen ? bitLen - 1 : endOffset; // BIT search return BitPosBitSearch(input, inputLen, startOffset, endOffset, searchFor); diff --git a/test/standalone/Garnet.test.complexstring/GarnetBitmapTests.cs b/test/standalone/Garnet.test.complexstring/GarnetBitmapTests.cs index 98409fc5555..e10cc46419e 100644 --- a/test/standalone/Garnet.test.complexstring/GarnetBitmapTests.cs +++ b/test/standalone/Garnet.test.complexstring/GarnetBitmapTests.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Linq; using System.Numerics.Tensors; +using System.Runtime.InteropServices; using System.Threading.Tasks; using Garnet.common; using Garnet.server; @@ -2408,6 +2409,76 @@ public void BitmapBitPosLongOffsetBoundaryValidationTest() ClassicAssert.AreEqual(-1, pos); } + /// + /// Regression test for an off-by-one clamp in BitPosDriver's BYTE-mode path: an explicit end + /// offset greater than or equal to the value's length was clamped to inputLen instead of + /// inputLen - 1, allowing BitPosByteSearch to read one byte past the end of the value + /// when no matching bit exists within the real data (mirrors the fix applied to BitCountDriver + /// in #1138, which was never mirrored to BITPOS). + /// + /// Calls BitmapManager.BitPosDriver directly against an unmanaged buffer with a known "canary" + /// byte placed immediately past the declared value length, so the out-of-bounds read is + /// deterministically observable: a matching bit hides in the canary byte, so the buggy driver + /// reports a real (non -1) position instead of -1, while the fixed driver never looks at it. + /// A plain end-to-end BITPOS call would rely on whatever bytes happen to follow the value on + /// the managed heap, which is not reliable enough to catch a one-byte overread. + /// + [Test] + [Category("BITPOS")] + public unsafe void BitmapBitPosOutOfBoundsEndOffsetByteModeTest() + { + const int valueLen = 8; + var buf = (byte*)NativeMemory.Alloc(valueLen + 1); + try + { + // Real value: no set bits anywhere within its declared bounds. + for (var i = 0; i < valueLen; i++) + buf[i] = 0x00; + + // Canary byte, one past the declared length. All bits set, so if the driver reads it + // while searching for a set bit, it will incorrectly report a position inside it. + buf[valueLen] = 0xFF; + + var pos = BitmapManager.BitPosDriver(buf, valueLen, startOffset: 0, endOffset: 1_000_000, searchFor: 1, offsetType: 0x0); + + ClassicAssert.AreEqual(-1, pos, "BITPOS BYTE mode must not read past the end of the value"); + } + finally + { + NativeMemory.Free(buf); + } + } + + /// + /// Same regression as , but for + /// BitPosDriver's BIT-mode path, whose equivalent clamp had the same off-by-one + /// (bitLen instead of bitLen - 1). + /// + [Test] + [Category("BITPOS")] + public unsafe void BitmapBitPosOutOfBoundsEndOffsetBitModeTest() + { + const int valueLen = 3; + var buf = (byte*)NativeMemory.Alloc(valueLen + 1); + try + { + // Real value: no set bits anywhere within its declared bounds (24 valid bit positions). + for (var i = 0; i < valueLen; i++) + buf[i] = 0x00; + + // Canary byte, one past the declared length. All bits set. + buf[valueLen] = 0xFF; + + var pos = BitmapManager.BitPosDriver(buf, valueLen, startOffset: 0, endOffset: 1_000_000, searchFor: 1, offsetType: 0x1); + + ClassicAssert.AreEqual(-1, pos, "BITPOS BIT mode must not read past the end of the value"); + } + finally + { + NativeMemory.Free(buf); + } + } + [Test] [Category("BITPOS")] public void BitmapBitPosBitModifierRequiresStartAndEndTest() From e34f09afbc0e3e773376d187bb52b121abad260a Mon Sep 17 00:00:00 2001 From: hexonal Date: Thu, 23 Jul 2026 00:45:45 -0400 Subject: [PATCH 2/2] Fix stale comment claiming startOffset > endOffset returns 0 Both the BYTE-mode and BIT-mode early-return branches return -1, not 0 - the comment was just wrong, no behavior change. --- libs/server/Resp/Bitmap/BitmapManagerBitPos.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/server/Resp/Bitmap/BitmapManagerBitPos.cs b/libs/server/Resp/Bitmap/BitmapManagerBitPos.cs index c0dea36c0f4..e425fc14c1e 100644 --- a/libs/server/Resp/Bitmap/BitmapManagerBitPos.cs +++ b/libs/server/Resp/Bitmap/BitmapManagerBitPos.cs @@ -28,7 +28,7 @@ public static long BitPosDriver(byte* input, int inputLen, long startOffset, lon if (startOffset >= inputLen) // If startOffset greater that valLen always bitpos -1 return -1; - if (startOffset > endOffset) // If start offset beyond endOffset return 0 + if (startOffset > endOffset) // If start offset beyond endOffset return -1 return -1; endOffset = endOffset >= inputLen ? inputLen - 1 : endOffset; @@ -47,7 +47,7 @@ public static long BitPosDriver(byte* input, int inputLen, long startOffset, lon if (startByteIndex >= inputLen) // If startOffset greater that valLen always bitpos -1 return -1; - if (startByteIndex > endByteIndex) // If start offset beyond endOffset return 0 + if (startByteIndex > endByteIndex) // If start offset beyond endOffset return -1 return -1; endOffset = endByteIndex >= inputLen ? bitLen - 1 : endOffset;