[PERF][med] Incremental STATUS_CHECK reads the ENTIRE old file (<=64MiB) before the size/mtime quick-check; wasted I/O on every unchanged file + amplification DoS
#255
Where: src/shared/file_receive.c:419-456 (unconditional protocol_alloc + read loop at 421-435; match decision at 440-456).
Problem: For every STATUS_CHECK the receiver reads the whole old file (up to 64MiB) BEFORE comparing size/mtime. Needed by no one in the common cases: (1) skip path (unchanged file) - read freed unused; (2) sendfile/incremental mode (no checksum, no delta) - content unused; (3) checksum mode where sizes differ. In multithreaded mode this stalls the network receive thread. Re-syncing a mostly-unchanged tree reads every unchanged file fully. Also a short-circuit bug at :446 when allocation fails (spurious mismatch).
Fix: fstat first; if size differs -> skip read; if size equal and !checksum -> decide by metadata_mtime_matches without reading; only read when actually needed (checksum+size-equal, or delta attempt).
Benchmark/verify incremental resync no longer reads skipped files.
Found by perf + security review of `dev` (9f8b588).
**Where:** src/shared/file_receive.c:419-456 (unconditional protocol_alloc + read loop at 421-435; match decision at 440-456).
**Problem:** For every STATUS_CHECK the receiver reads the whole old file (up to 64MiB) BEFORE comparing size/mtime. Needed by no one in the common cases: (1) skip path (unchanged file) - read freed unused; (2) sendfile/incremental mode (no checksum, no delta) - content unused; (3) checksum mode where sizes differ. In multithreaded mode this stalls the network receive thread. Re-syncing a mostly-unchanged tree reads every unchanged file fully. Also a short-circuit bug at :446 when allocation fails (spurious mismatch).
**Fix:** fstat first; if size differs -> skip read; if size equal and !checksum -> decide by metadata_mtime_matches without reading; only read when actually needed (checksum+size-equal, or delta attempt).
Benchmark/verify incremental resync no longer reads skipped files.
Fixed on dev via fix/receiver-correctness (14c064a): STATUS_CHECK now stats first and only reads the old file when a checksum/delta comparison actually needs it; allocation-failure path no longer spuriously mismatches. Unit tests added.
CI run #461: all jobs green (lint, build-and-test, sanitizers address+undefined, fuzz-build, coverage, valgrind). Unit 25/25, integration 83 passed / 10 skipped / 1 xpassed. Closing.
**Resolved on `dev`** (HEAD f7c6c91).
Fixed on dev via fix/receiver-correctness (14c064a): STATUS_CHECK now stats first and only reads the old file when a checksum/delta comparison actually needs it; allocation-failure path no longer spuriously mismatches. Unit tests added.
CI run #461: all jobs green (lint, build-and-test, sanitizers address+undefined, fuzz-build, coverage, valgrind). Unit 25/25, integration 83 passed / 10 skipped / 1 xpassed. Closing.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Found by perf + security review of
dev(9f8b588).Where: src/shared/file_receive.c:419-456 (unconditional protocol_alloc + read loop at 421-435; match decision at 440-456).
Problem: For every STATUS_CHECK the receiver reads the whole old file (up to 64MiB) BEFORE comparing size/mtime. Needed by no one in the common cases: (1) skip path (unchanged file) - read freed unused; (2) sendfile/incremental mode (no checksum, no delta) - content unused; (3) checksum mode where sizes differ. In multithreaded mode this stalls the network receive thread. Re-syncing a mostly-unchanged tree reads every unchanged file fully. Also a short-circuit bug at :446 when allocation fails (spurious mismatch).
Fix: fstat first; if size differs -> skip read; if size equal and !checksum -> decide by metadata_mtime_matches without reading; only read when actually needed (checksum+size-equal, or delta attempt).
Benchmark/verify incremental resync no longer reads skipped files.
Resolved on
dev(HEADf7c6c91).Fixed on dev via fix/receiver-correctness (
14c064a): STATUS_CHECK now stats first and only reads the old file when a checksum/delta comparison actually needs it; allocation-failure path no longer spuriously mismatches. Unit tests added.CI run #461: all jobs green (lint, build-and-test, sanitizers address+undefined, fuzz-build, coverage, valgrind). Unit 25/25, integration 83 passed / 10 skipped / 1 xpassed. Closing.