The PR makes substantial improvements across 41 files (+2112/−180 lines). It adds 12 new test suites (file_sendfile, glob, log, multiprocessing, scanner, transport_ssh/tcp/tls), fixes multiple memory safety and protocol issues, adds symlink support, IPv6 dual-stack, graceful server shutdown, server-side exclude/include filtering, config serialization, and security hardening.
Memory Safety
Issue
Severity
Status
malloc(0) → UB in data_create_empty
High
FIXED (data.c:7) — allocates at least 1 byte
Metadata wire format used platform-dependent types (mode_t, uid_t, time_t)
Medium
FIXED (metadata.c + metadata.h) — now int32_t/int64_t for cross-platform binary protocol
Compression level out of range → zstd UB
Medium
FIXED (compression.c:14-22) — clamped to [1, 22]
Decompressed size > SIZE_MAX → truncated malloc
High
FIXED (compression.c:83-91) — check added, fallback to growing buffer
SSH argv[16] stack buffer overflow
High
FIXED (transport_ssh.c:128) — dynamic calloc(32, ...) with bounds checks
FIXED (protocol.c:88-90, 116-118) — retry on non-fatal SSL errors
config_create NULL check missing after malloc
Medium
FIXED (config.c:18-19)
Scanner deep-copied exclude/include patterns instead of shallow ref
Low
FIXED (scanner.c:39-89) — avoids use-after-free if caller frees originals
Security
Issue
Severity
Status
Symlink path traversal (absolute link_target or ..)
High
FIXED (file.c:205-209, 213-217) — blocks link_target[0]=='/' and strstr(link_target, "..")
SUID/SGID privilege escalation via metadata restore
High
FIXED (metadata.c:154) — strips `S_ISUID
Server SIGTERM handler called _exit(0) without cleanup
Medium
FIXED (server.c:157-158) — uses server_request_shutdown() for graceful cleanup
file_save_to_disk weak path traversal check
Low
NOT FIXED — still uses strstr(disk_path, "..") which has false positives (valid..name) and potential bypasses (.../.../). Consider realpath() + prefix check.
No hostname verification in TLS client without CA
Low
PARTIAL — CA verification works with --ca, but without it, SSL_VERIFY_NONE logs a warning only (transport_tls.c:79-81). Acceptable for tooling.
Thread Safety
Issue
Severity
Status
io_read_fd, io_write_fd, io_ssl, io_bwlimit are __thread
OK
Correct — each thread gets its own copy
g_tcp_cleanup_requested is volatile sig_atomic_t
OK
Sufficient for single-writer (signal handler) + single-reader (main loop)
g_server_cleanup_requested same pattern
OK
Same analysis
accept_loop uses SIGCHLD=SIG_IGN
OK
No zombie processes
Logic / Correctness
Issue
Severity
Status
receive_delta_file: unexpected resp (neither DELTA_DATA nor NEXT) silently returns NULL without STATUS_ERROR
Low
Unreachable under correct protocol; could cause caller to hang
receive_incremental_check: send_status(STATUS_OK) failure on match returns NULL without STATUS_ERROR
Low
Same — caller may hang on I/O error
is_excluded on server creates str_dup + basename() for every file
Low
Minor perf: basename modifies input, so duplication is necessary
file_save_to_disk: strstr(disk_path, "..") is not NULL-safe
Low
disk_path is never NULL at that point
Code Quality
Strength
Detail
Thorough error path cleanup
Every goto error / early return frees all prior allocations
Deep copy in scanner
Previously stored caller-owned pointers; now owns copies → no UAF
config_receive overflow guard
SIZE_MAX / sizeof(char*) check prevents integer overflow in malloc
MAX_PATTERN_COUNT = 10000
Prevents resource exhaustion from malicious server
0 critical issues, 2 minor protocol error-path concerns
Major improvements in memory safety, protocol hardening, and cross-platform correctness
Recommend addressing the 2 minor protocol error-path issues in a follow-up, and adding missing test coverage for the path traversal blocking and partial file features
## PR Review: fix/test-coverage (e62296d)
### Verdict: **APPROVE with minor concerns**
The PR makes substantial improvements across 41 files (+2112/−180 lines). It adds 12 new test suites (file_sendfile, glob, log, multiprocessing, scanner, transport_ssh/tcp/tls), fixes multiple memory safety and protocol issues, adds symlink support, IPv6 dual-stack, graceful server shutdown, server-side exclude/include filtering, config serialization, and security hardening.
---
### Memory Safety
| Issue | Severity | Status |
|---|---|---|
| `malloc(0)` → UB in `data_create_empty` | High | **FIXED** (`data.c:7`) — allocates at least 1 byte |
| Metadata wire format used platform-dependent types (`mode_t`, `uid_t`, `time_t`) | Medium | **FIXED** (`metadata.c` + `metadata.h`) — now `int32_t`/`int64_t` for cross-platform binary protocol |
| Compression level out of range → zstd UB | Medium | **FIXED** (`compression.c:14-22`) — clamped to `[1, 22]` |
| Decompressed size `> SIZE_MAX` → truncated `malloc` | High | **FIXED** (`compression.c:83-91`) — check added, fallback to growing buffer |
| SSH `argv[16]` stack buffer overflow | High | **FIXED** (`transport_ssh.c:128`) — dynamic `calloc(32, ...)` with bounds checks |
| `bw_throttle` `elapsed_ns` potential overflow (`time_t * 1e9`) | Medium | **FIXED** (`protocol.c:39-41`) — uses `unsigned long long` + explicit casts |
| `bw_throttle` `deficit_ns` negative → `nanosleep` with huge negative value | Medium | **FIXED** (`protocol.c:53-54`) — clamped to 0 |
| `send_str(NULL)` → `strlen(NULL)` crash | High | **FIXED** (`protocol.c:156-159`) — NULL check + log + return false |
| `receive_str` no size limit → OOM | High | **FIXED** (`protocol.c:173-177`) — `MAX_STRING_SIZE` (10 MB) limit |
| `receive_data` no size limit → OOM | High | **FIXED** (`protocol.c:206-209`) — `MAX_DATA_SIZE` (1 GB) limit |
| SSL `WANT_READ`/`WANT_WRITE` treated as fatal error | Medium | **FIXED** (`protocol.c:88-90, 116-118`) — retry on non-fatal SSL errors |
| `config_create` NULL check missing after `malloc` | Medium | **FIXED** (`config.c:18-19`) |
| Scanner deep-copied exclude/include patterns instead of shallow ref | Low | **FIXED** (`scanner.c:39-89`) — avoids use-after-free if caller frees originals |
### Security
| Issue | Severity | Status |
|---|---|---|
| Symlink path traversal (absolute `link_target` or `..`) | High | **FIXED** (`file.c:205-209, 213-217`) — blocks `link_target[0]=='/'` and `strstr(link_target, "..")` |
| SUID/SGID privilege escalation via metadata restore | High | **FIXED** (`metadata.c:154`) — strips `S_ISUID|S_ISGID` in `chmod` |
| Server SIGTERM handler called `_exit(0)` without cleanup | Medium | **FIXED** (`server.c:157-158`) — uses `server_request_shutdown()` for graceful cleanup |
| `file_save_to_disk` weak path traversal check | Low | **NOT FIXED** — still uses `strstr(disk_path, "..")` which has false positives (`valid..name`) and potential bypasses (`.../.../`). Consider `realpath()` + prefix check. |
| No hostname verification in TLS client without CA | Low | **PARTIAL** — CA verification works with `--ca`, but without it, `SSL_VERIFY_NONE` logs a warning only (`transport_tls.c:79-81`). Acceptable for tooling. |
### Thread Safety
| Issue | Severity | Status |
|---|---|---|
| `io_read_fd`, `io_write_fd`, `io_ssl`, `io_bwlimit` are `__thread` | OK | Correct — each thread gets its own copy |
| `g_tcp_cleanup_requested` is `volatile sig_atomic_t` | OK | Sufficient for single-writer (signal handler) + single-reader (main loop) |
| `g_server_cleanup_requested` same pattern | OK | Same analysis |
| `accept_loop` uses `SIGCHLD=SIG_IGN` | OK | No zombie processes |
### Logic / Correctness
| Issue | Severity | Status |
|---|---|---|
| `receive_delta_file`: unexpected `resp` (neither `DELTA_DATA` nor `NEXT`) silently returns NULL without `STATUS_ERROR` | Low | Unreachable under correct protocol; could cause caller to hang |
| `receive_incremental_check`: `send_status(STATUS_OK)` failure on match returns NULL without `STATUS_ERROR` | Low | Same — caller may hang on I/O error |
| `is_excluded` on server creates `str_dup` + `basename()` for every file | Low | Minor perf: `basename` modifies input, so duplication is necessary |
| `file_save_to_disk`: `strstr(disk_path, "..")` is not `NULL`-safe | Low | `disk_path` is never NULL at that point |
### Code Quality
| Strength | Detail |
|---|---|
| Thorough error path cleanup | Every `goto error` / early return frees all prior allocations |
| Deep copy in scanner | Previously stored caller-owned pointers; now owns copies → no UAF |
| `config_receive` overflow guard | `SIZE_MAX / sizeof(char*)` check prevents integer overflow in malloc |
| `MAX_PATTERN_COUNT` = 10000 | Prevents resource exhaustion from malicious server |
| Metadata wire format fix | Fixed-size integers ensure cross-platform protocol compatibility |
| IPv6 dual-stack | `server_create` tries `AF_INET6` first, falls back to `AF_INET4` |
| `server_accept_loop` public | Enables TLS to use the same accept loop as plain TCP |
| `client_connect` uses `getaddrinfo` | Proper IPv4/IPv6 resolution instead of raw `inet_pton` |
| Symlink support | New `FileType` enum, `link_target` field, `FILE_TYPE_SYMLINK` handling in scanner/send/receive/save |
### Test Coverage Analysis
| Category | Coverage |
|---|---|
| **New test suites** | `test_file_sendfile`, `test_glob`, `test_log`, `test_multiprocessing`, `test_scanner`, `test_transport_ssh`, `test_transport_tcp`, `test_transport_tls` |
| **Edge cases covered** | Empty file sendfile, `malloc(0)` data, oversized string rejection, truncated read, NULL destroy, empty directory scanner, globstar patterns, compression fallback |
| **Missing tests** | `directory_scanner_create_full` with `follow_symlinks=false`, partial file recovery, `io_set_bwlimit`/bw_throttle, server-side `is_excluded`, `delete_extras`/`receive_manifest`, `file_save_to_disk` symlink path traversal blocking, `file_send_streaming` error paths, `receive_delta_file`, `send_str(NULL)` regression |
### Summary
- **All 22 existing unit tests pass**
- Build is clean (no warnings with default flags)
- **0 critical issues**, 2 minor protocol error-path concerns
- Major improvements in memory safety, protocol hardening, and cross-platform correctness
- Recommend addressing the 2 minor protocol error-path issues in a follow-up, and adding missing test coverage for the path traversal blocking and partial file features
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.
Adds unit tests for:\n- #71: sendfile zero-copy\n- #63: multiprocessing module\n- #62: logging module\n- #56: scanner edge cases\n- #55: transport modules
PR Review: fix/test-coverage (
e62296d)Verdict: APPROVE with minor concerns
The PR makes substantial improvements across 41 files (+2112/−180 lines). It adds 12 new test suites (file_sendfile, glob, log, multiprocessing, scanner, transport_ssh/tcp/tls), fixes multiple memory safety and protocol issues, adds symlink support, IPv6 dual-stack, graceful server shutdown, server-side exclude/include filtering, config serialization, and security hardening.
Memory Safety
malloc(0)→ UB indata_create_emptydata.c:7) — allocates at least 1 bytemode_t,uid_t,time_t)metadata.c+metadata.h) — nowint32_t/int64_tfor cross-platform binary protocolcompression.c:14-22) — clamped to[1, 22]> SIZE_MAX→ truncatedmalloccompression.c:83-91) — check added, fallback to growing bufferargv[16]stack buffer overflowtransport_ssh.c:128) — dynamiccalloc(32, ...)with bounds checksbw_throttleelapsed_nspotential overflow (time_t * 1e9)protocol.c:39-41) — usesunsigned long long+ explicit castsbw_throttledeficit_nsnegative →nanosleepwith huge negative valueprotocol.c:53-54) — clamped to 0send_str(NULL)→strlen(NULL)crashprotocol.c:156-159) — NULL check + log + return falsereceive_strno size limit → OOMprotocol.c:173-177) —MAX_STRING_SIZE(10 MB) limitreceive_datano size limit → OOMprotocol.c:206-209) —MAX_DATA_SIZE(1 GB) limitWANT_READ/WANT_WRITEtreated as fatal errorprotocol.c:88-90, 116-118) — retry on non-fatal SSL errorsconfig_createNULL check missing aftermallocconfig.c:18-19)scanner.c:39-89) — avoids use-after-free if caller frees originalsSecurity
link_targetor..)file.c:205-209, 213-217) — blockslink_target[0]=='/'andstrstr(link_target, "..")metadata.c:154) — strips `S_ISUID_exit(0)without cleanupserver.c:157-158) — usesserver_request_shutdown()for graceful cleanupfile_save_to_diskweak path traversal checkstrstr(disk_path, "..")which has false positives (valid..name) and potential bypasses (.../.../). Considerrealpath()+ prefix check.--ca, but without it,SSL_VERIFY_NONElogs a warning only (transport_tls.c:79-81). Acceptable for tooling.Thread Safety
io_read_fd,io_write_fd,io_ssl,io_bwlimitare__threadg_tcp_cleanup_requestedisvolatile sig_atomic_tg_server_cleanup_requestedsame patternaccept_loopusesSIGCHLD=SIG_IGNLogic / Correctness
receive_delta_file: unexpectedresp(neitherDELTA_DATAnorNEXT) silently returns NULL withoutSTATUS_ERRORreceive_incremental_check:send_status(STATUS_OK)failure on match returns NULL withoutSTATUS_ERRORis_excludedon server createsstr_dup+basename()for every filebasenamemodifies input, so duplication is necessaryfile_save_to_disk:strstr(disk_path, "..")is notNULL-safedisk_pathis never NULL at that pointCode Quality
goto error/ early return frees all prior allocationsconfig_receiveoverflow guardSIZE_MAX / sizeof(char*)check prevents integer overflow in mallocMAX_PATTERN_COUNT= 10000server_createtriesAF_INET6first, falls back toAF_INET4server_accept_looppublicclient_connectusesgetaddrinfoinet_ptonFileTypeenum,link_targetfield,FILE_TYPE_SYMLINKhandling in scanner/send/receive/saveTest Coverage Analysis
test_file_sendfile,test_glob,test_log,test_multiprocessing,test_scanner,test_transport_ssh,test_transport_tcp,test_transport_tlsmalloc(0)data, oversized string rejection, truncated read, NULL destroy, empty directory scanner, globstar patterns, compression fallbackdirectory_scanner_create_fullwithfollow_symlinks=false, partial file recovery,io_set_bwlimit/bw_throttle, server-sideis_excluded,delete_extras/receive_manifest,file_save_to_disksymlink path traversal blocking,file_send_streamingerror paths,receive_delta_file,send_str(NULL)regressionSummary
Re-review: FIX VERIFIED ✅
Verdict: APPROVED
Pull request closed