Reject integer overflow of length fields in zipmapValidateIntegrity by madolson · Pull Request #3920 · valkey-io/valkey

madolson · 2026-06-04T21:55:45Z

Problem

A crafted zipmap entry can set the value length to a value near UINT32_MAX so that the l + e sum (value length + one-byte free space) wraps in unsigned int arithmetic. The wrapped sum advances the validation cursor by a tiny amount, leaving p inside the buffer, so the OUT_OF_RANGE check passes and zipmapValidateIntegrity wrongly returns success. The field-length path has the same shape — advancing p by a ~4GB length wraps the pointer on 32-bit builds.

zipmapValidateIntegrity is always called with deep=1 from rdb.c when loading RDB_TYPE_HASH_ZIPMAP, including via RESTORE, so any client with RESTORE access can submit a payload that passes validation. On 32-bit platforms this leads to out-of-bounds access during the subsequent zipmap→listpack conversion. On 64-bit the downstream lpSafeToAdd cap happens to reject it (the raw ~4GB length exceeds LISTPACK_MAX_SAFETY_SIZE), but the validator should not accept a malformed payload in the first place — this is the function whose sole job is to reject it.

Fix

Bounds-check the attacker-controlled length against the bytes remaining in the zipmap, in 64-bit space, before any pointer arithmetic, for both the field-length and value-length paths.

Testing

tests/integration/corrupt-dump.tcl: a RESTORE-path test exercising the full attack surface; asserts rejection and that the server stays up.
Verified the test fails on the pre-fix code (validator accepts the value-length payload) and passes after the fix, confirmed by stashing the fix during the integration run.
Full integration/corrupt-dump suite: 76 passed, 0 failed.

Note

Found via structure-aware fuzzing of the RESTORE path. This issue was generated by AI but verified, with love, by a human.

coderabbitai · 2026-06-04T21:56:02Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d400b70-532c-43fd-ad6a-bd0283f87792

📥 Commits

Reviewing files that changed from the base of the PR and between ee87798 and d687651.

📒 Files selected for processing (2)

src/zipmap.c
tests/integration/corrupt-dump.tcl

🚧 Files skipped from review as they are similar to previous changes (2)

tests/integration/corrupt-dump.tcl
src/zipmap.c

📝 Walkthrough

Walkthrough

This PR adds two 64-bit bounds checks to zipmapValidateIntegrity() to prevent integer wraparound in field and value-length calculations, and an integration test that verifies a corrupted zipmap restore is rejected and the server remains responsive.

Changes

Zipmap integer overflow validation

Layer / File(s)	Summary
Core validation bounds checks `src/zipmap.c`	Two new 64-bit overflow guards check field-name length and combined value-length calculations before advancing the parsing pointer, preventing wrap-around that could bypass subsequent range validation.
Integration test for corrupted zipmap payload `tests/integration/corrupt-dump.tcl`	End-to-end test with checksum validation disabled crafts a malformed zipmap, asserts restore failure with "Bad data format", confirms "Zipmap integrity check failed" appears in logs, and verifies server health via ping.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding validation to reject integer overflow of length fields in the zipmapValidateIntegrity function, which is the primary purpose of this security-focused PR.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, explaining the security problem, the fix implemented, and testing approach for validating the zipmap length field overflow vulnerability.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-06-04T21:56:08Z

❌ Provenance Check Alert

Potential code similarities detected with upstream repository.

2026-06-04 21:56:08 [INFO] - matches redis/redis PR #9633 (similarity: 0.933, method: file_simhash+deep); file pairs: tests/integration/corrupt-dump.tcl <- tests/integration/corrupt-dump.tcl
2026-06-04 21:56:08 [INFO] - matches redis/redis PR #15251 (similarity: 0.978, method: file_simhash+deep); file pairs: tests/integration/corrupt-dump.tcl <- tests/integration/corrupt-dump.tcl

This check was performed automatically by the Provenance Guard Action.

github-actions · 2026-06-04T21:59:20Z

❌ Provenance Check Alert

Potential code similarities detected with upstream repository.

2026-06-04 21:59:19 [INFO] - matches redis/redis PR #9633 (similarity: 0.933, method: file_simhash+deep); file pairs: tests/integration/corrupt-dump.tcl <- tests/integration/corrupt-dump.tcl
2026-06-04 21:59:19 [INFO] - matches redis/redis PR #15251 (similarity: 0.978, method: file_simhash+deep); file pairs: tests/integration/corrupt-dump.tcl <- tests/integration/corrupt-dump.tcl

This check was performed automatically by the Provenance Guard Action.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/zipmap.c`:
- Around line 225-231: The code reads and increments p to fetch the free-byte e
without first ensuring that there is at least one byte left, which can consume
the ZIPMAP_END terminator; before executing "e = *p++" add a one-byte bounds
check against the buffer end (use zm and size), e.g. verify p < zm + size (or p
<= zm + size - 1) and return 0 on failure; keep the subsequent 64-bit check for
(uint64_t)l + e against (zm + size - 1 - p) as-is to prevent overflow.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bad41e44-5fc7-4b8a-a972-10c965091396

📥 Commits

Reviewing files that changed from the base of the PR and between c8c38e7 and 176ac00.

📒 Files selected for processing (3)

src/unit/test_zipmap.cpp
src/zipmap.c
tests/integration/corrupt-dump.tcl

codecov · 2026-06-04T22:25:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.69%. Comparing base (c8c38e7) to head (d687651).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3920      +/-   ##
============================================
+ Coverage     76.63%   76.69%   +0.05%     
============================================
  Files           162      162              
  Lines         80735    80737       +2     
============================================
+ Hits          61875    61923      +48     
+ Misses        18860    18814      -46

Files with missing lines	Coverage Δ
src/zipmap.c	`100.00% <100.00%> (ø)`

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

A crafted zipmap entry can set the value length to a value near UINT32_MAX so that the `l + e` sum (value length plus the one-byte free space) wraps in unsigned int arithmetic. The wrapped sum advances the validation cursor by a small amount, leaving it inside the buffer, so the OUT_OF_RANGE check passes and validation wrongly succeeds. The field-length path has the same shape: advancing `p` by a 4GB length wraps the pointer on 32-bit builds. zipmapValidateIntegrity is always called with deep=1 from rdb.c when loading RDB_TYPE_HASH_ZIPMAP, including via RESTORE, so a client with RESTORE access can submit a payload that passes validation. On 32-bit platforms this leads to out-of-bounds access during the subsequent zipmap-to-listpack conversion. On 64-bit the downstream lpSafeToAdd cap happens to reject it, but the validator must not accept it in the first place. Bounds-check the attacker-controlled length against the bytes remaining in the zipmap, in 64-bit space, before doing any pointer arithmetic. Add a corrupt-dump integration test covering the value-length overflow case through the RESTORE path. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

github-actions · 2026-06-05T03:42:55Z

❌ Provenance Check Alert

Potential code similarities detected with upstream repository.

2026-06-05 03:42:55 [INFO] - matches redis/redis PR #9633 (similarity: 0.933, method: file_simhash+deep); file pairs: tests/integration/corrupt-dump.tcl <- tests/integration/corrupt-dump.tcl
2026-06-05 03:42:55 [INFO] - matches redis/redis PR #15251 (similarity: 0.978, method: file_simhash+deep); file pairs: tests/integration/corrupt-dump.tcl <- tests/integration/corrupt-dump.tcl

This check was performed automatically by the Provenance Guard Action.

…3920) ## Problem A crafted zipmap entry can set the value length to a value near `UINT32_MAX` so that the `l + e` sum (value length + one-byte free space) wraps in `unsigned int` arithmetic. The wrapped sum advances the validation cursor by a tiny amount, leaving `p` inside the buffer, so the `OUT_OF_RANGE` check passes and `zipmapValidateIntegrity` wrongly returns success. The field-length path has the same shape — advancing `p` by a ~4GB length wraps the pointer on 32-bit builds. `zipmapValidateIntegrity` is always called with `deep=1` from `rdb.c` when loading `RDB_TYPE_HASH_ZIPMAP`, **including via `RESTORE`**, so any client with `RESTORE` access can submit a payload that passes validation. On 32-bit platforms this leads to out-of-bounds access during the subsequent zipmap→listpack conversion. On 64-bit the downstream `lpSafeToAdd` cap happens to reject it (the raw ~4GB length exceeds `LISTPACK_MAX_SAFETY_SIZE`), but the validator should not accept a malformed payload in the first place — this is the function whose sole job is to reject it. ## Fix Bounds-check the attacker-controlled length against the bytes remaining in the zipmap, in 64-bit space, **before** any pointer arithmetic, for both the field-length and value-length paths. ## Testing - `tests/integration/corrupt-dump.tcl`: a `RESTORE`-path test exercising the full attack surface; asserts rejection and that the server stays up. - Verified the test **fails on the pre-fix code** (validator accepts the value-length payload) and **passes after the fix**, confirmed by stashing the fix during the integration run. - Full `integration/corrupt-dump` suite: 76 passed, 0 failed. > [!NOTE] > Found via structure-aware fuzzing of the RESTORE path. This issue was generated by AI but verified, with love, by a human. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

…3920) A crafted zipmap entry can set the value length to a value near `UINT32_MAX` so that the `l + e` sum (value length + one-byte free space) wraps in `unsigned int` arithmetic. The wrapped sum advances the validation cursor by a tiny amount, leaving `p` inside the buffer, so the `OUT_OF_RANGE` check passes and `zipmapValidateIntegrity` wrongly returns success. The field-length path has the same shape — advancing `p` by a ~4GB length wraps the pointer on 32-bit builds. `zipmapValidateIntegrity` is always called with `deep=1` from `rdb.c` when loading `RDB_TYPE_HASH_ZIPMAP`, **including via `RESTORE`**, so any client with `RESTORE` access can submit a payload that passes validation. On 32-bit platforms this leads to out-of-bounds access during the subsequent zipmap→listpack conversion. On 64-bit the downstream `lpSafeToAdd` cap happens to reject it (the raw ~4GB length exceeds `LISTPACK_MAX_SAFETY_SIZE`), but the validator should not accept a malformed payload in the first place — this is the function whose sole job is to reject it. Bounds-check the attacker-controlled length against the bytes remaining in the zipmap, in 64-bit space, **before** any pointer arithmetic, for both the field-length and value-length paths. - `tests/integration/corrupt-dump.tcl`: a `RESTORE`-path test exercising the full attack surface; asserts rejection and that the server stays up. - Verified the test **fails on the pre-fix code** (validator accepts the value-length payload) and **passes after the fix**, confirmed by stashing the fix during the integration run. - Full `integration/corrupt-dump` suite: 76 passed, 0 failed. > [!NOTE] > Found via structure-aware fuzzing of the RESTORE path. This issue was generated by AI but verified, with love, by a human. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

…3920) ## Problem A crafted zipmap entry can set the value length to a value near `UINT32_MAX` so that the `l + e` sum (value length + one-byte free space) wraps in `unsigned int` arithmetic. The wrapped sum advances the validation cursor by a tiny amount, leaving `p` inside the buffer, so the `OUT_OF_RANGE` check passes and `zipmapValidateIntegrity` wrongly returns success. The field-length path has the same shape — advancing `p` by a ~4GB length wraps the pointer on 32-bit builds. `zipmapValidateIntegrity` is always called with `deep=1` from `rdb.c` when loading `RDB_TYPE_HASH_ZIPMAP`, **including via `RESTORE`**, so any client with `RESTORE` access can submit a payload that passes validation. On 32-bit platforms this leads to out-of-bounds access during the subsequent zipmap→listpack conversion. On 64-bit the downstream `lpSafeToAdd` cap happens to reject it (the raw ~4GB length exceeds `LISTPACK_MAX_SAFETY_SIZE`), but the validator should not accept a malformed payload in the first place — this is the function whose sole job is to reject it. ## Fix Bounds-check the attacker-controlled length against the bytes remaining in the zipmap, in 64-bit space, **before** any pointer arithmetic, for both the field-length and value-length paths. ## Testing - `tests/integration/corrupt-dump.tcl`: a `RESTORE`-path test exercising the full attack surface; asserts rejection and that the server stays up. - Verified the test **fails on the pre-fix code** (validator accepts the value-length payload) and **passes after the fix**, confirmed by stashing the fix during the integration run. - Full `integration/corrupt-dump` suite: 76 passed, 0 failed. > [!NOTE] > Found via structure-aware fuzzing of the RESTORE path. This issue was generated by AI but verified, with love, by a human. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

…3920) A crafted zipmap entry can set the value length to a value near `UINT32_MAX` so that the `l + e` sum (value length + one-byte free space) wraps in `unsigned int` arithmetic. The wrapped sum advances the validation cursor by a tiny amount, leaving `p` inside the buffer, so the `OUT_OF_RANGE` check passes and `zipmapValidateIntegrity` wrongly returns success. The field-length path has the same shape — advancing `p` by a ~4GB length wraps the pointer on 32-bit builds. `zipmapValidateIntegrity` is always called with `deep=1` from `rdb.c` when loading `RDB_TYPE_HASH_ZIPMAP`, **including via `RESTORE`**, so any client with `RESTORE` access can submit a payload that passes validation. On 32-bit platforms this leads to out-of-bounds access during the subsequent zipmap→listpack conversion. On 64-bit the downstream `lpSafeToAdd` cap happens to reject it (the raw ~4GB length exceeds `LISTPACK_MAX_SAFETY_SIZE`), but the validator should not accept a malformed payload in the first place — this is the function whose sole job is to reject it. Bounds-check the attacker-controlled length against the bytes remaining in the zipmap, in 64-bit space, **before** any pointer arithmetic, for both the field-length and value-length paths. - `tests/integration/corrupt-dump.tcl`: a `RESTORE`-path test exercising the full attack surface; asserts rejection and that the server stays up. - Verified the test **fails on the pre-fix code** (validator accepts the value-length payload) and **passes after the fix**, confirmed by stashing the fix during the integration run. - Full `integration/corrupt-dump` suite: 76 passed, 0 failed. > [!NOTE] > Found via structure-aware fuzzing of the RESTORE path. This issue was generated by AI but verified, with love, by a human. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

…3920) ## Problem A crafted zipmap entry can set the value length to a value near `UINT32_MAX` so that the `l + e` sum (value length + one-byte free space) wraps in `unsigned int` arithmetic. The wrapped sum advances the validation cursor by a tiny amount, leaving `p` inside the buffer, so the `OUT_OF_RANGE` check passes and `zipmapValidateIntegrity` wrongly returns success. The field-length path has the same shape — advancing `p` by a ~4GB length wraps the pointer on 32-bit builds. `zipmapValidateIntegrity` is always called with `deep=1` from `rdb.c` when loading `RDB_TYPE_HASH_ZIPMAP`, **including via `RESTORE`**, so any client with `RESTORE` access can submit a payload that passes validation. On 32-bit platforms this leads to out-of-bounds access during the subsequent zipmap→listpack conversion. On 64-bit the downstream `lpSafeToAdd` cap happens to reject it (the raw ~4GB length exceeds `LISTPACK_MAX_SAFETY_SIZE`), but the validator should not accept a malformed payload in the first place — this is the function whose sole job is to reject it. ## Fix Bounds-check the attacker-controlled length against the bytes remaining in the zipmap, in 64-bit space, **before** any pointer arithmetic, for both the field-length and value-length paths. ## Testing - `tests/integration/corrupt-dump.tcl`: a `RESTORE`-path test exercising the full attack surface; asserts rejection and that the server stays up. - Verified the test **fails on the pre-fix code** (validator accepts the value-length payload) and **passes after the fix**, confirmed by stashing the fix during the integration run. - Full `integration/corrupt-dump` suite: 76 passed, 0 failed. > [!NOTE] > Found via structure-aware fuzzing of the RESTORE path. This issue was generated by AI but verified, with love, by a human. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

github-actions Bot assigned madolson Jun 4, 2026

madolson force-pushed the fix-zipmap-validate-length-overflow branch from 176ac00 to ee87798 Compare June 4, 2026 21:58

coderabbitai Bot reviewed Jun 4, 2026

View reviewed changes

Comment thread src/zipmap.c

madolson force-pushed the fix-zipmap-validate-length-overflow branch from ee87798 to d687651 Compare June 5, 2026 03:42

madolson requested review from enjoy-binbin and murphyjacob4 June 5, 2026 04:35

enjoy-binbin approved these changes Jun 5, 2026

View reviewed changes

madolson added this to Valkey 9.1, Valkey 9.0, Valkey 8.1, Valkey 8.0 and Valkey 7.2 Jun 5, 2026

murphyjacob4 approved these changes Jun 5, 2026

View reviewed changes

madolson merged commit 63dc748 into valkey-io:unstable Jun 11, 2026
62 of 63 checks passed

github-project-automation Bot moved this to To be backported in Valkey 7.2 Jun 11, 2026

github-project-automation Bot moved this to To be backported in Valkey 9.1 Jun 11, 2026

github-project-automation Bot moved this to To be backported in Valkey 8.0 Jun 11, 2026

github-project-automation Bot moved this to To be backported in Valkey 9.0 Jun 11, 2026

github-project-automation Bot moved this to To be backported in Valkey 8.1 Jun 11, 2026

valkeyrie-ops Bot mentioned this pull request Jun 9, 2026

[backport] Backport sweep for 8.1 #3917

Open

valkeyrie-ops Bot mentioned this pull request Jun 11, 2026

[backport] Backport sweep for 9.1 #3970

Closed

valkeyrie-ops Bot mentioned this pull request Jun 9, 2026

[backport] Backport sweep for 7.2 #3738

Merged

valkeyrie-ops Bot mentioned this pull request Jun 9, 2026

[backport] Backport sweep for 8.0 #3736

Merged

valkeyrie-ops Bot mentioned this pull request Jun 15, 2026

[backport] Backport sweep for 9.1 #3987

Open

valkeyrie-ops Bot moved this from To be backported to Done in Valkey 7.2 Jun 17, 2026

valkeyrie-ops Bot moved this from To be backported to Done in Valkey 8.0 Jun 18, 2026

valkeyrie-ops Bot mentioned this pull request Jun 22, 2026

[backport] Backport sweep for 9.0 #3879

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reject integer overflow of length fields in zipmapValidateIntegrity#3920

Reject integer overflow of length fields in zipmapValidateIntegrity#3920
madolson merged 1 commit into
valkey-io:unstablefrom
madolson:fix-zipmap-validate-length-overflow

madolson commented Jun 4, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 4, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

github-actions Bot commented Jun 4, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov Bot commented Jun 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

madolson commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Testing

Uh oh!

coderabbitai Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jun 4, 2026

❌ Provenance Check Alert

Uh oh!

github-actions Bot commented Jun 4, 2026

❌ Provenance Check Alert

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jun 5, 2026

❌ Provenance Check Alert

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

madolson commented Jun 4, 2026 •

edited

Loading

coderabbitai Bot commented Jun 4, 2026 •

edited

Loading

codecov Bot commented Jun 4, 2026 •

edited

Loading