Skip to content

Validate replication RDB bulk lengths - #29

Draft
roshkhatri wants to merge 1 commit into
unstablefrom
replication-bulk-length
Draft

Validate replication RDB bulk lengths#29
roshkhatri wants to merge 1 commit into
unstablefrom
replication-bulk-length

Conversation

@roshkhatri

@roshkhatri roshkhatri commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Summary

Validates the RDB bulk-length metadata a replica receives from a primary before starting a full synchronization transfer, and keeps the transfer boundary intact on 32-bit builds.

Findings

  • The size-based header was parsed with unchecked strtol. A negative value such as $-1, signed overflow, or trailing garbage could reach later replication paths as a transfer size. In diskless loading this could trigger a server assertion and terminate the replica.
  • Parsing used strlen on a buffer whose real length was already known, so a NUL byte could hide the rest of the line. $1\0garbage was accepted as $1 and the replica proceeded to load an RDB.
  • $0 was accepted as a size-based length, but 0 is how this code represents an EOF-delimited transfer. With usemark false, disk-based sync retries zero-byte reads and diskless sync passes a zero read limit to RIO, which RIO treats as unlimited.
  • The size is stored in off_t (64-bit) but was passed to rioInitWithConn() as size_t. On 32-bit builds 2^32 narrows to 0, which again means unlimited, so the declared boundary disappeared. The RIO limit arithmetic could also overflow.

Fix

  • Parse the complete decimal field with string2ll using the known length, and reject negative, zero, overflowed, and trailing-garbage values.
  • Reject an interior NUL and stop using C-string functions on the metadata line.
  • Make the RIO connection read limit and counter uint64_t, and phrase the limit checks as subtraction so they cannot wrap.
  • Preserve EOF-delimited synchronization and valid large RDB transfers, without applying the client request bulk limit.

Compatibility

No DUMP or RDB format change and no wire protocol change. Real primaries send canonical decimal lengths, so ordinary replication including across versions is unaffected. A custom primary sending $+1, $01, or a length with stray whitespace will now fail the sync.

Validation

  • Full build passed.
  • Full integration/replication suite: 77 passed.
  • Fake-primary coverage for $-1, $0, signed overflow, trailing garbage, embedded NUL, a valid control, and the 2^32 boundary.
  • Each rejection case was checked against the unpatched code. The embedded-NUL and $0 cases fail there, the latter by hanging the replica rather than rejecting.
  • git diff --check and clang-format 18 source checks passed.
  • The 32-bit narrowing is verified by types and inspection only. Upstream CI has build-32bit but no 32-bit test run, and -m32 was unavailable locally.

Draft in the fork for manual security review and backport assessment.

Signed-off-by: Roshan Khatri <roshanvkhatri@gmail.com>
@roshkhatri
roshkhatri force-pushed the replication-bulk-length branch from c5a527f to d0f089e Compare August 11, 2026 20:12
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.

1 participant