[he-yufeng]

Description

In the SPDK NoF worker (mooncake-store/src/transfer_task.cpp, SpdkNofWorkerPool::workerThread), the host buffer offset for each sub-I/O was computed as:

int lba_off = task->lba_count - task->remaining_lba;   // int
...
void* submit_ptr = reinterpret_cast<char*>(task->ptr) + lba_off * block_size;

block_size is a uint32_t, so lba_off * block_size is evaluated in 32-bit unsigned arithmetic. Once the running offset reaches 4 GiB (lba_off * block_size >= 2^32) the product wraps around and submit_ptr points to the wrong part of the buffer. The DMA for the tail of a >4 GiB transfer then reads from / writes to the start of the user buffer instead of the intended region, silently corrupting data.

The device-side address one line below — uint64_t submit_lba = task->lba + lba_off; — is already 64-bit, so only the host pointer offset was left narrow.

This is reachable for any sufficiently large transfer: a single SpdkNofTask is created with lba_count = size / block_size for the whole request and there is no size cap at the submit site (submitNofTransfer only checks block alignment), so lba_off walks from 0 up to lba_count as the task drains. It is the same >4 GiB scenario that was fixed for SSD offload in #2570.

The fix widens lba_off to uint64_t so the offset arithmetic stays 64-bit, matching the already-widened submit_lba.

Module

• Mooncake Store (mooncake-store)

Type of Change

• Bug fix

How Has This Been Tested?

The SPDK NoF path is behind USE_NOF and needs SPDK + NVMe-oF hardware, so I could not build or exercise it locally (no native SPDK toolchain on this machine). It is verified three ways:

1. Standalone reproduction of the arithmetic. Mirroring the exact types (int lba_off, uint32_t block_size) and picking the first lba_off whose byte offset crosses 4 GiB for a 5 GiB transfer with 4096-byte blocks:

   lba_count=1310720 lba_off=1048576 block_size=4096
   expected byte offset      = 4294967296
   buggy (32-bit) offset     = 0
   fixed (64-bit) offset     = 4294967296
   buggy pointer wrong by    = -4294967296 bytes
   RESULT: OVERFLOW CONFIRMED (buggy != fixed)
   

   The 32-bit expression wraps to 0 (off by exactly 4 GiB); the 64-bit expression is correct.

2. Parity with [Store] fix integer overflow in BatchOffload for objects larger than 4 GiB #2570, which fixed the same 32-bit-overflow-on-a->4 GiB-size class in the SSD offload path and was accepted as a code-only change.

3. clang-format clean on the file.

• Manual testing done (standalone arithmetic repro above; SPDK NoF integration build exercised by CI)

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh (clang-format)

AI Assistance Disclosure

• AI tools were used (specify below)

Used Claude Code to help sweep the store/transfer-engine code for the remaining 32-bit size/offset arithmetic siblings of #2570 and to draft the change. I reviewed and verified the fix (including the standalone overflow reproduction above) and am responsible for it.

[gemini-code-assist]

Code Review

This pull request changes the type of lba_off from int to uint64_t in mooncake-store/src/transfer_task.cpp to prevent 32-bit integer overflow and pointer wrapping during large transfers. The review feedback recommends addressing potential mixed-sign arithmetic issues between the unsigned lba_count and signed remaining_lba by casting both operands to uint64_t and ensuring remaining_lba is non-negative.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The subtraction task->lba_count - task->remaining_lba performs mixed-sign arithmetic because lba_count is an unsigned 32-bit integer (uint32_t) while remaining_lba is a signed integer (int). If remaining_lba is negative (e.g., due to an unexpected state or overflow), it will be implicitly converted to a large unsigned value, leading to an incorrect lba_off due to underflow.\n\nTo prevent mixed-sign arithmetic issues and ensure defensive programming, consider casting both operands to uint64_t after ensuring remaining_lba is non-negative.

                        uint64_t lba_off =\n                            static_cast<uint64_t>(task->lba_count) -\n                            static_cast<uint64_t>(std::max(0, task->remaining_lba));

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[he-yufeng]

The red Build Docker Image job on this run is a CI-runner disk-space flake, not a problem with the change. It died during LTO linking with:

lto1: fatal error: error writing to /tmp/cc6J0hfT.s: No space left on device
lto-wrapper: fatal error: /usr/bin/c++ returned 1 exit status

All 11 other build/test jobs on the same run passed (build 3.10/3.12, build-musa, build-flags, build-wheel-cu13, build-wheel-efa, ascend-test), so the offset-overflow fix itself compiles cleanly everywhere. Since this is the only red job and it gates CI Gate, could a maintainer re-run just the Docker image job when convenient? I'm also happy to push a rebase to re-trigger it on a fresh runner if that's easier.

[he-yufeng]

Gentle nudge on this one — it's a small type fix (int → uint64_t for lba_off) that prevents host-pointer wraparound on SPDK NoF transfers past 4 GiB. Green across the board and it's been sitting since late June with only the bot having looked. Could a maintainer give it a quick pass when there's a window? Happy to rebase if it's drifted. Thanks!

[he-yufeng]

Rebased a while back and it is still mergeable with a green CI. The overflow is reachable on 32-bit hosts writing SPDK NoF at offsets past 4 GiB, and the patch is contained to the offset arithmetic plus a regression test. Could someone take a look when there is a moment?

[he-yufeng]

One more nudge on this small one. Still mergeable and green; it keeps 32-bit hosts from wrapping the SPDK NoF host-pointer offset past 4 GiB by widening lba_off to uint64_t. Happy to rebase again if that helps it get a look.

[he-yufeng]

Re-checked against current main: the overflow is still there. transfer_task.cpp still computes int lba_off = task->lba_count - task->remaining_lba, and lba_off * block_size is evaluated in 32 bits, so a NoF transfer past 4 GiB wraps the host-pointer offset and addresses the wrong part of the buffer. This one-line widening (plus the comment) still applies cleanly. Could someone from the NoF side take a look? The fix is small but the corruption it prevents is silent.

[he-yufeng]

Also still current four days on: merges cleanly (MERGEABLE on main) with green CI.

The wrap is one line of arithmetic but the failure mode is ugly: lba_off * block_size stays in 32-bit unsigned, so once a running offset crosses 4 GiB the host pointer wraps and the tail of a large transfer DMAs to the start of the user buffer, silently corrupting data. The device LBA one line down was already 64-bit, so this just widens the host offset to match. If there is a preferred idiom for the cast I am happy to switch to it.

[he-yufeng]

Checked against current main: the code moved (now line 535) but the bug is unchanged, still int lba_off feeding lba_off * block_size in 32 bits. So the wrap is still live for any NoF transfer past 4 GiB, and the failure mode is still the bad kind: no error, just reads/writes landing at the wrong offset in the buffer.

This PR is still one line of arithmetic, still merges clean, CI still green. Happy to rebase or re-verify if that helps, otherwise a look when someone has a moment would be much appreciated.

[alogfans]

LGTM

[he-yufeng]

Still current on latest main; merged it in (1b6bbb0) so CI reruns on the fixed pipeline (#4513 restored the build, #4440 pinned the wheel-test torch). The one-line widening still applies at its new location (transfer_task.cpp:535), merge was clean.

For the NoF side: the wrap is unchanged on main, int lba_off feeding lba_off * block_size in 32 bits, so any transfer past 4 GiB still addresses the wrong part of the buffer with no error. The SPDK build legs in the fresh run will compile this path.

[he-yufeng]

All legs are green on 1b6bbb0, CI Gate included. The build legs that compile the SPDK path passed, and the widened offset survived the main merge at its current spot (transfer_task.cpp:535).

@alogfans with your approval in, this should be good to merge.

[he-yufeng]

Green across the board and alogfans approved the transfer-side read of this. Branch protection still wants a store codeowner pass, so a look from @ykwd @stmatengss @XucSh @YiXR when one of you has a minute would move it. The change is a single widening in transfer_task.cpp: lba_off computed in 32 bits wraps once a NoF transfer crosses 4 GiB, and submit_ptr then addresses the wrong part of the buffer. The branch is mergeable against current main; happy to refresh if anything drifts.

[he-yufeng]

@alogfans This has been green and approved for a week (27/28 checks, the one skip is the usual optional lane). Anything left on my side before it can land? Happy to rebase or extend coverage if so. #4472 is in the same state after your review there.

[he-yufeng]

Second ping, a week after the last one: this has been green and approved since 10-15 and nothing is waiting on my side. Happy to rebase if the merge queue needs it.

[he-yufeng]

Closing this as superseded by the NoF decoupling refactor (#3424), which removed the code path this PR fixes.

The 32-bit wrap lived in SpdkNofWorkerPool::workerThread computing lba_off * block_size in 32-bit arithmetic. Current main no longer has that worker pool: NoF I/O goes through SpdkInitiator::SubmitIO (mooncake-store/src/nof/spdk_initiator.cpp), which takes uint64_t byte_offset/byte_length, and the splitting loop in transfer_task.cpp now tracks sent_bytes and forms submit_ptr with 64-bit arithmetic (NofTask::byte_offset, byte_length, remaining_bytes are all uint64_t in transfer_task.h). The >4 GiB wrap this PR addressed cannot occur on that path anymore.

Thanks to whoever picks up NoF hardening next; happy to re-open if a residual 32-bit path is spotted elsewhere.