[S2.14] Fix DEBUG DIGEST crash on compressed values#43
Merged
Conversation
DEBUG DIGEST and DEBUG DIGEST-VALUE iterate the kvstore directly (computeDatasetDigest via kvstoreIteratorNext; DIGEST-VALUE via dbFind) so a debug command can digest logically-expired keys. They therefore bypass the lookupKey() transient-view decompression hook and observe raw OBJ_ENCODING_COMPRESSED robjs. Both funnel a string value through xorObjectDigest's OBJ_STRING branch into mixStringObjectDigest / xorStringObjectDigest, which called getDecodedObject() — that panics on any encoding other than RAW/EMBSTR/INT (serverPanic 'Unknown encoding type'), crashing the server on the first compressed value. Route the two digest helpers through objectGetUncompressedView (R2.5.2), the single decoder primitive PR #24 used for rdbSaveStringObject / rioWriteBulkObject. For a compressed value the helper decompresses into a scratch sds (read directly — not via getDecodedObject, since incrRefCount on the OBJ_STATIC_REFCOUNT view would panic); otherwise it falls back to getDecodedObject (still handles INT). No-op passthrough for non-compressed values and for BUILD_ZSTD=no. The digest is thus defined over logical (decompressed) bytes: identical whether a value is stored compressed or not. Pre-existing crash on unstable; a blocker for transparency mode (full Tcl corpus under --compression, which runs DEBUG DIGEST extensively). Test tests/unit/compression-debug-readers.tcl (external:skip): compressed values -> DEBUG DIGEST / DIGEST-VALUE don't crash, digest is byte-identical compressed vs decompressed, and a DEBUG RELOAD round-trip preserves both digest and values.
Mark S2.14 done in plan.md (the follow-up to the S2.13 audit finding) and list DEBUG DIGEST / DIGEST-VALUE among R2.5.7's explicit-decompression kvstore-direct readers in the design doc.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a pre-existing crash:
DEBUG DIGEST/DEBUG DIGEST-VALUEpanic the server on anyOBJ_ENCODING_COMPRESSEDvalue. The follow-up to the S2.13 audit finding; a blocker for transparency mode (§7.1 — the full Tcl corpus under--compressionrunsDEBUG DIGESTfor consistency/replication checks). Product fix indebug.c+ oneexternal:skiptest. No design behavior change.Root cause
The DEBUG DIGEST family deliberately reads the kvstore directly so it can digest logically-expired keys —
computeDatasetDigestviakvstoreIteratorNext,DEBUG DIGEST-VALUEviadbFind— bypassing thelookupKey()transient-view decompression hook. Both funnel a string value throughxorObjectDigest'sOBJ_STRINGbranch intomixStringObjectDigest/xorStringObjectDigest, which calledgetDecodedObject(). That panics on any encoding other than RAW/EMBSTR/INT (serverPanic("Unknown encoding type"), object.c) → crash on the first compressed value.Same class PR #24 fixed for
rdbSaveStringObject/rioWriteBulkObject, but the digest helpers were missed.Fix
Route
mixStringObjectDigestandxorStringObjectDigestthroughobjectGetUncompressedView(R2.5.2) — the single decoder primitive, used exactly as PR #24 used it. For a compressed value the helper decompresses into a scratch sds, which is digested directly (not viagetDecodedObject—incrRefCounton theOBJ_STATIC_REFCOUNTview would panic); otherwise it falls back togetDecodedObject(still handles INT). No-op passthrough for non-compressed values and forBUILD_ZSTD=no.Consequence: the digest is defined over logical (decompressed) bytes — byte-identical whether the value is stored compressed or not.
Audit (full kvstore-direct reader sweep)
Only the two digest paths crash. Audited-safe:
OBJECT ENCODING/REFCOUNT/…objectCommandLookuppassesLOOKUP_NO_BYTES→strEncodingreturns "compressed"DEBUG SDSLEN!sdsEncodedObject→ "Not an sds encoded string"DEBUG OBJECTstrEncoding+rdbSavedObjectLen(PR#24-fixed path)getDecodedObjecton argv (AOF argv, moduleRM_Call, pubsub, cluster, LCS, t_list elements, sort, bitop sources)lookupKey'd to RAWdbUnshareStringValue(db.c)lookupKeyWritedecompressed first (S2.13)Deferred (not crashes, separate owners):
LOOKUP_NO_BYTES) →objectComputeSizesees RAW → no crash, but reports the uncompressed size (R2.7.3 wants compressed). That's S3.5 (@GilboaAWS).dictID/compressedlength/uncompressedlengthfields — a feature gap, separate.Test
tests/unit/compression-debug-readers.tcl(external:skip, like the COW test — it churns global compression state):DEBUG DIGESTover 20 compressed keys doesn't crash; digest is byte-identical compressed vs. decompressed (transparency).DEBUG DIGEST-VALUEon a compressed key doesn't crash; identical to its uncompressed digest.DEBUG RELOADround-trip preserves both the digest and the values.Verification
BUILD_ZSTD={yes,no}both build clean with-Werror.unit/other(the digest path that crashed pre-fix) andunit/type/compressionpass, server alive.DEBUG DIGEST→Guru Meditation object.c:945; post-fix → identical digest compressed vs decompressed, server alive.Docs
detailed-design.mdR2.5.7: listsDEBUG DIGEST/DIGEST-VALUEamong the kvstore-direct readers that callobjectGetUncompressedViewexplicitly.plan.md: S2.14 marked done.