- Fix transport_tcp.c: initialize server->ssl_ctx to NULL
(prevents SSL_CTX_free on garbage when server_create_tls fails)
- Fix test_transport_tcp.c: client_create now sets fd=-1, family=AF_UNSPEC
- Fix test_transport_tcp.c: server address family may be AF_INET or AF_INET6
- Fix test_file_sendfile.c: send_path=false protocol includes file_type prefix
✅ Conflicting flag detection (sendfile+compression, incremental+chunk-serialization, etc.)
Issues Found
MEDIUM: mkdir_r bounds check may fail for the last path component (false pathologically)
In utils.c:39 — the check pos + part_len + 1 >= buf_size is correct for all practical paths, but it could theoretically reject a valid path if the sum of components and separators exactly equals path_len and the trailing null is the issue. The check uses >= rather than >, meaning it requires at least 1 byte of slack. For a path like /a (len=2, buf_size=4): pos starts at 1, part "a" has part_len=1, check 1+1+1=3 >= 4 → false (OK). This is fine in practice.
LOW: test_scanner_max_size creates files before directory exists (duplicate writes)
tests/test_scanner.c lines 251-256:
create_test_file(small,"tiny");// writes before mkdir
create_test_file(large,"this...");// writes before mkdir
mkdir(dir,0755);create_test_file(small,"tiny");// writes again
create_test_file(large,"this...");// writes again
The first two writes succeed via mkdir_r as a side effect, then the explicit mkdir is a no-op (EEXIST), then files are written again. No crash or correctness issue, but redundant.
LOW: Symlink path traversal prevention is filename-only
file.c:205: the check on link_target blocks ".." and absolute paths, but a chain of symlinks on the receiving filesystem could still escape the root directory. This is inherent to symlinks and acceptable for the intended use case.
INFO: Global g_server / g_tcp_cleanup_requested race window
In server.c:226-252: there's a small race between server_create and server_listen where a SIGINT could set g_server_cleanup_requested but the server would still enter server_listen. However, accept_loop checks the flag and exits immediately, so this is harmless.
Commit Structure
The 12 commits are well-scoped:
7ecba4e — Adapt tests/fixes from enhancements rebase
9e3e0f5 — ci: trigger CI on PR
3923421 — Review fixes (getsockname, test assertion, memcpy, clang-format)
266369b/a91267c/70e1c67 — CI re-triggers
e2a500e — Remove redundant free before _exit in transport_ssh.c
5d819f7 — Test quality: cppcheck suppressions, TLS test addresses, log test isolation
cdd6d1c — clang-format compliance
Conclusion
All changes are correct, safe, and well-tested. The PR is ready to merge after addressing the minor comments above.
## PR #90 Review — Branch: `fix/enhancements` (SHA: cdd6d1c)
**Verdict: APPROVE with minor comments**
---
### Summary
This PR is large and well-structured (12 commits) covering multiple categories:
| Category | Scope |
|---|---|
| **Security** | Path traversal prevention, TLS hostname verification, SUID/SGID stripping, MAX_STRING_SIZE/MAX_DATA_SIZE bounds, OOM guards |
| **Memory safety** | NULL checks after all malloc/calloc calls, `strcpy`→`memcpy` migration, bounds-checked buffer writes |
| **Protocol safety** | Size limits on `receive_str` (10 MB) and `receive_data` (1 GB), integer overflow checks in `config_receive` |
| **Reliability** | EINTR handling in accept loop, graceful shutdown flag, bandwidth throttle overflow fix |
| **Infrastructure** | Dual-stack IPv6/IPv4 sockets, globstar (`**`) pattern support, thread-local I/O state |
| **Tests** | New tests for sendfile, log, multiprocessing, transport_ssh, transport_tcp, transport_tls, globstar, scanner patterns, oversized protocol values |
---
### Changed Files & Findings
#### 1. `src/shared/file.c` — Path traversal, sendfile, delta transfers
- ✅ Path traversal blocked in both `file_save_to_disk` (symlink targets) and `receive_incremental_check`
- ✅ SUID/SGID stripped in `file_restore_metadata` (via `metadata.c`)
- ✅ `sendfile` in `file_send_sendfile` properly returns early for compression fallback
- ✅ All malloc → NULL checks in `file_create`, `receive_delta_file`, `receive_incremental_check`, etc.
- ✅ Complete cleanup on all error paths in `receive_delta_file`
#### 2. `src/shared/transport_tcp.c` — IPv6, EINTR, graceful shutdown
- ✅ Dual-stack socket creation with automatic IPv4 fallback
- ✅ `accept_loop` now handles EINTR correctly (retry unless shutdown requested)
- ✅ `server_request_shutdown` with `volatile sig_atomic_t` flag
- ✅ Socket timeouts (SO_RCVTIMEO/SO_SNDTIMEO) set on accepted connections
- ✅ `client_create` no longer opens a socket eagerly (deferred to `client_connect`)
- ✅ `client_connect` uses `getaddrinfo` with AF_UNSPEC for proper DNS resolution
- ✅ `client_disconnect` checks `file_descriptor >= 0` before close
#### 3. `src/shared/transport_tls.c` — Hostname verification, SNI
- ✅ SNI set via `SSL_set_tlsext_host_name`
- ✅ Hostname verification via `X509_VERIFY_PARAM_set1_host`
- ✅ `SSL_get_verify_result` checked in client mode
- ✅ Warning logged when CA not provided (client falls to `SSL_VERIFY_NONE`)
- ✅ `TLS1_2_VERSION` minimum protocol
#### 4. `src/shared/transport_ssh.c` — Bounded argv, address type fix
- ✅ Fixed `address.ss_family = AF_UNIX` (was using `sin_family` on sockaddr_storage)
- ✅ Replaced fixed `ssh_argv[16]` with `calloc(ssh_argv_max, ...)` — bounded dynamic allocation
- ✅ Bounds checks before indexing into `ssh_argv` array
- ✅ `free(ssh_argv)` before `_exit` in exec failure path
#### 5. `src/shared/protocol.c` — Size limits, SSL-aware I/O
- ✅ `send_str` checks for NULL data
- ✅ `receive_str` enforces `MAX_STRING_SIZE` (10 MB)
- ✅ `receive_data` enforces `MAX_DATA_SIZE` (1 GB)
- ✅ SSL-aware `send_n_data`/`receive_n_data` with `SSL_ERROR_WANT_*` handling
- ✅ Bandwidth throttle uses `unsigned long long` to avoid overflow
#### 6. `src/shared/config.c` — Integer overflow guards
- ✅ `MAX_PATTERN_COUNT` (10000) limit in `config_receive`
- ✅ `(size_t)ec > SIZE_MAX / sizeof(char*)` overflow check before malloc
- ✅ Same protections for both exclude and include patterns
- ✅ Clean `goto error` cleanup path frees all partially-allocated resources
#### 7. `src/shared/utils.c` — memcpy, globstar, bounds checking
- ✅ `strcpy` completely eliminated in favor of `memcpy`
- ✅ `mkdir_r` now checks buffer bounds (`pos + part_len + 1 >= buf_size`)
- ✅ Globstar (`**`) pattern matching support
- ✅ `glob_match` handles `/**/` as zero-or-more path component separator
#### 8. `src/client/scanner.c` — NULL checks, exclusion patterns
- ✅ All malloc NULL checks in `directory_scanner_create_full`
- ✅ Clean rollback of partial allocations on failure
- ✅ `cur_path` is always freed on all code paths (no leak)
- ✅ Symlink handling with `follow_symlinks` correctly resolves target
#### 9. `src/client/client_cli.c` — Argument validation
- ✅ Port validation (1-65535) for both SSH and server ports
- ✅ `--bwlimit` overflow check (`kbps > ULLONG_MAX / 1024`)
- ✅ `--delta-block` / `--delta-max` range validation
- ✅ Conflicting flag detection (sendfile+compression, incremental+chunk-serialization, etc.)
---
### Issues Found
#### MEDIUM: `mkdir_r` bounds check may fail for the last path component (false pathologically)
In `utils.c:39` — the check `pos + part_len + 1 >= buf_size` is correct for all practical paths, but it could theoretically reject a valid path if the sum of components and separators exactly equals `path_len` and the trailing null is the issue. The check uses `>=` rather than `>`, meaning it requires at least 1 byte of slack. For a path like `/a` (len=2, buf_size=4): pos starts at 1, part "a" has part_len=1, check `1+1+1=3 >= 4` → false (OK). This is fine in practice.
#### LOW: `test_scanner_max_size` creates files before directory exists (duplicate writes)
`tests/test_scanner.c` lines 251-256:
```c
create_test_file(small, "tiny"); // writes before mkdir
create_test_file(large, "this..."); // writes before mkdir
mkdir(dir, 0755);
create_test_file(small, "tiny"); // writes again
create_test_file(large, "this..."); // writes again
```
The first two writes succeed via `mkdir_r` as a side effect, then the explicit `mkdir` is a no-op (EEXIST), then files are written again. No crash or correctness issue, but redundant.
#### LOW: Symlink path traversal prevention is filename-only
`file.c:205`: the check on `link_target` blocks `".."` and absolute paths, but a chain of symlinks on the receiving filesystem could still escape the root directory. This is inherent to symlinks and acceptable for the intended use case.
#### INFO: Global `g_server` / `g_tcp_cleanup_requested` race window
In `server.c:226-252`: there's a small race between `server_create` and `server_listen` where a SIGINT could set `g_server_cleanup_requested` but the server would still enter `server_listen`. However, `accept_loop` checks the flag and exits immediately, so this is harmless.
---
### Commit Structure
The 12 commits are well-scoped:
1. `7ecba4e` — Adapt tests/fixes from enhancements rebase
2. `9e3e0f5` — ci: trigger CI on PR
3. `3923421` — Review fixes (getsockname, test assertion, memcpy, clang-format)
4. `266369b/a91267c/70e1c67` — CI re-triggers
5. `e2a500e` — Remove redundant free before _exit in transport_ssh.c
6. `9dd925a` — accept_loop race condition, auto-detect valgrind, cppcheck suppressions
7. `224e7e8` — Security: path traversal, TLS hostname, SUID, OOM, stack overflow
8. `3ffc5c5` — Memory safety: malloc NULL checks, strcpy→memcpy, str_dup NULL check
9. `5d819f7` — Test quality: cppcheck suppressions, TLS test addresses, log test isolation
10. `cdd6d1c` — clang-format compliance
---
### Conclusion
All changes are correct, safe, and well-tested. The PR is ready to merge after addressing the minor comments above.
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.
Implements:\n- #70/#36: TCP timeout/keepalive\n- #34: Symlink handling\n- #33: IPv6 support\n- #32: TLS cert verification\n- #57: Filter transmission\n- #40: Streaming for large files\n- #37: Partial transfer resume
PR #90 Review — Branch:
fix/enhancements(SHA:cdd6d1c)Verdict: APPROVE with minor comments
Summary
This PR is large and well-structured (12 commits) covering multiple categories:
strcpy→memcpymigration, bounds-checked buffer writesreceive_str(10 MB) andreceive_data(1 GB), integer overflow checks inconfig_receive**) pattern support, thread-local I/O stateChanged Files & Findings
1.
src/shared/file.c— Path traversal, sendfile, delta transfersfile_save_to_disk(symlink targets) andreceive_incremental_checkfile_restore_metadata(viametadata.c)sendfileinfile_send_sendfileproperly returns early for compression fallbackfile_create,receive_delta_file,receive_incremental_check, etc.receive_delta_file2.
src/shared/transport_tcp.c— IPv6, EINTR, graceful shutdownaccept_loopnow handles EINTR correctly (retry unless shutdown requested)server_request_shutdownwithvolatile sig_atomic_tflagclient_createno longer opens a socket eagerly (deferred toclient_connect)client_connectusesgetaddrinfowith AF_UNSPEC for proper DNS resolutionclient_disconnectchecksfile_descriptor >= 0before close3.
src/shared/transport_tls.c— Hostname verification, SNISSL_set_tlsext_host_nameX509_VERIFY_PARAM_set1_hostSSL_get_verify_resultchecked in client modeSSL_VERIFY_NONE)TLS1_2_VERSIONminimum protocol4.
src/shared/transport_ssh.c— Bounded argv, address type fixaddress.ss_family = AF_UNIX(was usingsin_familyon sockaddr_storage)ssh_argv[16]withcalloc(ssh_argv_max, ...)— bounded dynamic allocationssh_argvarrayfree(ssh_argv)before_exitin exec failure path5.
src/shared/protocol.c— Size limits, SSL-aware I/Osend_strchecks for NULL datareceive_strenforcesMAX_STRING_SIZE(10 MB)receive_dataenforcesMAX_DATA_SIZE(1 GB)send_n_data/receive_n_datawithSSL_ERROR_WANT_*handlingunsigned long longto avoid overflow6.
src/shared/config.c— Integer overflow guardsMAX_PATTERN_COUNT(10000) limit inconfig_receive(size_t)ec > SIZE_MAX / sizeof(char*)overflow check before mallocgoto errorcleanup path frees all partially-allocated resources7.
src/shared/utils.c— memcpy, globstar, bounds checkingstrcpycompletely eliminated in favor ofmemcpymkdir_rnow checks buffer bounds (pos + part_len + 1 >= buf_size)**) pattern matching supportglob_matchhandles/**/as zero-or-more path component separator8.
src/client/scanner.c— NULL checks, exclusion patternsdirectory_scanner_create_fullcur_pathis always freed on all code paths (no leak)follow_symlinkscorrectly resolves target9.
src/client/client_cli.c— Argument validation--bwlimitoverflow check (kbps > ULLONG_MAX / 1024)--delta-block/--delta-maxrange validationIssues Found
MEDIUM:
mkdir_rbounds check may fail for the last path component (false pathologically)In
utils.c:39— the checkpos + part_len + 1 >= buf_sizeis correct for all practical paths, but it could theoretically reject a valid path if the sum of components and separators exactly equalspath_lenand the trailing null is the issue. The check uses>=rather than>, meaning it requires at least 1 byte of slack. For a path like/a(len=2, buf_size=4): pos starts at 1, part "a" has part_len=1, check1+1+1=3 >= 4→ false (OK). This is fine in practice.LOW:
test_scanner_max_sizecreates files before directory exists (duplicate writes)tests/test_scanner.clines 251-256:The first two writes succeed via
mkdir_ras a side effect, then the explicitmkdiris a no-op (EEXIST), then files are written again. No crash or correctness issue, but redundant.LOW: Symlink path traversal prevention is filename-only
file.c:205: the check onlink_targetblocks".."and absolute paths, but a chain of symlinks on the receiving filesystem could still escape the root directory. This is inherent to symlinks and acceptable for the intended use case.INFO: Global
g_server/g_tcp_cleanup_requestedrace windowIn
server.c:226-252: there's a small race betweenserver_createandserver_listenwhere a SIGINT could setg_server_cleanup_requestedbut the server would still enterserver_listen. However,accept_loopchecks the flag and exits immediately, so this is harmless.Commit Structure
The 12 commits are well-scoped:
7ecba4e— Adapt tests/fixes from enhancements rebase9e3e0f5— ci: trigger CI on PR3923421— Review fixes (getsockname, test assertion, memcpy, clang-format)266369b/a91267c/70e1c67— CI re-triggerse2a500e— Remove redundant free before _exit in transport_ssh.c9dd925a— accept_loop race condition, auto-detect valgrind, cppcheck suppressions224e7e8— Security: path traversal, TLS hostname, SUID, OOM, stack overflow3ffc5c5— Memory safety: malloc NULL checks, strcpy→memcpy, str_dup NULL check5d819f7— Test quality: cppcheck suppressions, TLS test addresses, log test isolationcdd6d1c— clang-format complianceConclusion
All changes are correct, safe, and well-tested. The PR is ready to merge after addressing the minor comments above.
Re-review: FIX VERIFIED ✅
Verdict: APPROVED