kvcache-ai / kvcache-ai/Mooncake

[Bug]: File-per-key path mapping can collide and return another object's data

Open
#2,980 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
6.6k
Forks
1.2k
Avg merge
3d 5h
Merged PRs (30d)
312

Description

## Bug Report

On current `main` (`f20b7061`), `ResolvePathFromKey()` is not collision-safe for file-per-key storage.

The path uses only the low 8 bits of `std::hash` for its two directory levels, while `SanitizeKey()` maps each of `/\\:*?"<>|` to `_`. For a fixed tenant, the 10,000 four-character keys built from those characters plus `_` all sanitize to the same filename but have only 256 possible directory pairs, so at least two distinct keys must map to the same path.

This is a correctness problem because:

- writes open the destination with `O_TRUNC`, so the later key overwrites the earlier file;
- `StorageBackendAdaptor::BatchLoad()` deserializes the stored `KVEntry` without checking that its key matches the requested key;
- when the colliding keys and values have equal lengths, a read can silently return the other key's value.

The same path helper is used by file-per-key LOCAL_DISK storage and the legacy DISK replica path.

## Expected behavior

Distinct object identities must never resolve to the same on-disk object path in normal operation, and reads must reject records whose stored key does not match the requested key.

## Proposed direction

Use a stable full-key digest for the filename, retain a legacy-path read fallback for existing files, and validate the deserialized key before copying data to the caller.

### Before submitting...

- [x] Searched existing issues and open PRs for path/key collision reports.

Contributor guide

Open the contributing guide

Research direction

Locate ResolvePathFromKey(), SanitizeKey(), and StorageBackendAdaptor::BatchLoad(), then trace their LOCAL_DISK and legacy DISK callers. Reproduce the described colliding keys, inspect existing storage tests if available, and confirm that distinct keys receive distinct paths, legacy files remain readable, and BatchLoad rejects a stored key mismatch.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
62/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.