This PR performs a major cleanup and hardening of the transport layer and ancillary modules. It adds IPv4/IPv6 dual-stack support (via getaddrinfo / sockaddr_storage), a sendfile-based zero-copy file transfer path, globstar (**) pattern matching, graceful server shutdown, TLS SNI + hostname verification, exclude/include pattern filtering on the server side, and a comprehensive test suite expansion. 40 files changed, ~1,000 lines added.
✅ Positive findings
Area
Detail
IPv6
server_create and client_connect both try IPv6 first, fall back to IPv4 via getaddrinfo + sockaddr_storage.
Memory safety
mkdir_r replaced strcpy with memcpy and added bounds checking; str_dup now uses memcpy.
Client now sets SNI (SSL_set_tlsext_host_name) and hostname verification (X509_VERIFY_PARAM_set1_host). SSL_get_verify_result is checked after handshake.
Symlink safety
file_save_to_disk blocks absolute symlink targets and .. in target; also rejects .. in disk_path.
SUID/SGID stripping
file_restore_metadata masks `S_ISUID
Config receive hardening
config_receive caps pattern count at MAX_PATTERN_COUNT (10,000) and checks for integer overflow (src/shared/config.c:258-266).
Graceful shutdown
server_request_shutdown() + volatile sig_atomic_t flag terminates the accept loop cleanly on SIGINT/SIGTERM.
Deep copy of patterns
directory_scanner_create_full deep-copies exclude/include patterns and cleans up on allocation failure.
Compression clamping
data_compress clamps level to [1, 22].
data_create_empty(0)
Allocates at least 1 byte to avoid UB from malloc(0).
Test coverage
7 new test suites (test_file_sendfile, test_log, test_multiprocessing, test_transport_tcp, test_transport_ssh, test_transport_tls, test_scanner edge cases). All 22 tests pass.
Valgrind compatibility
test_file_sendfile and test_file skip fork-based tests under valgrind using /proc/self/maps heuristic.
Uses strstr on disk_path. This flags any path containing the substring .., including legitimate names like foo..bar.txt. While conservative (safe against ../), it causes false rejections. A better approach: scan for .. as a complete path component (e.g., tokenize by /).
Severity: Low (conservative false-positive, not a vulnerability).
3. Bandwidth limiter not thread-safe (src/shared/protocol.c:15-17)
These are not__thread, so they're shared across threads. bw_tokens is read/written without atomics in bw_throttle(). Pre-existing; not introduced by this PR.
Severity: Low-Medium (pre-existing; could cause inaccurate throttling under multithreaded transfers, but not a memory-safety issue).
4. is_running_under_valgrind() reads at most 4095 bytes (tests/test_utils.h)
On a system with an unusually large /proc/self/maps, the vgpreload string could appear after the first 4095 bytes, causing a false negative. In practice, vgpreload always appears near the top, so this is unlikely to trigger.
Severity: Very Low.
5. client_connect uses rp after break — but this is safe (src/shared/transport_tcp.c:268)
for(rp=res;rp!=NULL;rp=rp->ai_next){fd=socket(...);...if(connect(fd,rp->ai_addr,rp->ai_addrlen)==0)break;...}// rp used here
memcpy(&client->address,rp->ai_addr,copy_len);
rp is valid because the loop exits via break only when connect succeeded, at which point rp points to the current addrinfo node. If no connection succeeded, the fd < 0 check returns false before this point. No bug — but this is a subtle invariant worth documenting with a comment.
Severity: None (correct, but subtle).
✅ Verdict
APPROVED. The PR is well-structured, solidly tested (22/22 passing), and introduces significant hardening (IPv6, TLS hostname verification, SUID/SGID stripping, protocol bounds, config receive overflow protection, graceful shutdown). The issues found are all low-severity:
Issue #1 (EINTR in sendfile) and issue #2 (strstr("..") false positives) are worth fixing before merge but are not blockers.
Issue #3 (bandwidth limiter thread safety) is pre-existing and orthogonal to this PR.
## PR Review: `fix/integration-cleanup` (sha `b20e390`)
### Overview
This PR performs a major cleanup and hardening of the transport layer and ancillary modules. It adds IPv4/IPv6 dual-stack support (via `getaddrinfo` / `sockaddr_storage`), a `sendfile`-based zero-copy file transfer path, globstar (`**`) pattern matching, graceful server shutdown, TLS SNI + hostname verification, exclude/include pattern filtering on the server side, and a comprehensive test suite expansion. 40 files changed, ~1,000 lines added.
---
### ✅ Positive findings
| Area | Detail |
|------|--------|
| **IPv6** | `server_create` and `client_connect` both try IPv6 first, fall back to IPv4 via `getaddrinfo` + `sockaddr_storage`. |
| **Memory safety** | `mkdir_r` replaced `strcpy` with `memcpy` and added bounds checking; `str_dup` now uses `memcpy`. |
| **Protocol hardening** | `receive_str` enforces `MAX_STRING_SIZE` (10 MB); `receive_data` enforces `MAX_DATA_SIZE` (1 GB). |
| **TLS** | Client now sets SNI (`SSL_set_tlsext_host_name`) and hostname verification (`X509_VERIFY_PARAM_set1_host`). `SSL_get_verify_result` is checked after handshake. |
| **Symlink safety** | `file_save_to_disk` blocks absolute symlink targets and `..` in target; also rejects `..` in `disk_path`. |
| **SUID/SGID stripping** | `file_restore_metadata` masks `S_ISUID | S_ISGID` (`src/shared/metadata.c:154`). |
| **Config receive hardening** | `config_receive` caps pattern count at `MAX_PATTERN_COUNT` (10,000) and checks for integer overflow (`src/shared/config.c:258-266`). |
| **Graceful shutdown** | `server_request_shutdown()` + `volatile sig_atomic_t` flag terminates the `accept` loop cleanly on `SIGINT`/`SIGTERM`. |
| **Deep copy of patterns** | `directory_scanner_create_full` deep-copies exclude/include patterns and cleans up on allocation failure. |
| **Compression clamping** | `data_compress` clamps level to `[1, 22]`. |
| **`data_create_empty(0)`** | Allocates at least 1 byte to avoid UB from `malloc(0)`. |
| **Test coverage** | 7 new test suites (`test_file_sendfile`, `test_log`, `test_multiprocessing`, `test_transport_tcp`, `test_transport_ssh`, `test_transport_tls`, `test_scanner` edge cases). All 22 tests pass. |
| **Valgrind compatibility** | `test_file_sendfile` and `test_file` skip fork-based tests under valgrind using `/proc/self/maps` heuristic. |
---
### ⚠️ Issues found
#### 1. `sendfile` missing `EINTR` retry (`src/shared/file.c:626-631`)
```c
while ((unsigned long long)offset < file_size) {
ssize_t sent = sendfile(file_descriptor, fd, &offset, file_size - offset);
if (sent == -1) {
perror("sendfile failed");
close(fd);
return false;
}
}
```
`sendfile(2)` can return `-1` with `errno == EINTR` on interruption. This will cause an unnecessary transfer failure. Should retry on `EINTR`.
**Severity:** Low (rare in practice; signal handler installs are minimal in this codebase).
---
#### 2. Overly broad path traversal check (`src/shared/file.c:213, 229`)
```c
if (strstr(disk_path, "..") != NULL) {
log_message(LOG_LEVEL_ERROR, "Path traversal blocked: %s", disk_path);
...
}
```
Uses `strstr` on `disk_path`. This flags any path containing the substring `..`, including legitimate names like `foo..bar.txt`. While conservative (safe against `../`), it causes false rejections. A better approach: scan for `..` as a complete path component (e.g., tokenize by `/`).
**Severity:** Low (conservative false-positive, not a vulnerability).
---
#### 3. Bandwidth limiter not thread-safe (`src/shared/protocol.c:15-17`)
```c
static unsigned long long io_bwlimit = 0;
static long long bw_tokens = 0;
static struct timespec bw_last_refill = {0, 0};
```
These are **not** `__thread`, so they're shared across threads. `bw_tokens` is read/written without atomics in `bw_throttle()`. Pre-existing; not introduced by this PR.
**Severity:** Low-Medium (pre-existing; could cause inaccurate throttling under multithreaded transfers, but not a memory-safety issue).
---
#### 4. `is_running_under_valgrind()` reads at most 4095 bytes (`tests/test_utils.h`)
```c
size_t n = fread(buf, 1, sizeof(buf) - 1, f);
buf[n] = '\0';
return strstr(buf, "vgpreload") != NULL;
```
On a system with an unusually large `/proc/self/maps`, the `vgpreload` string could appear after the first 4095 bytes, causing a false negative. In practice, `vgpreload` always appears near the top, so this is unlikely to trigger.
**Severity:** Very Low.
---
#### 5. `client_connect` uses `rp` after `break` — but this is safe (`src/shared/transport_tcp.c:268`)
```c
for (rp = res; rp != NULL; rp = rp->ai_next) {
fd = socket(...);
...
if (connect(fd, rp->ai_addr, rp->ai_addrlen) == 0)
break;
...
}
// rp used here
memcpy(&client->address, rp->ai_addr, copy_len);
```
`rp` is valid because the loop exits via `break` only when `connect` succeeded, at which point `rp` points to the current `addrinfo` node. If no connection succeeded, the `fd < 0` check returns false before this point. **No bug** — but this is a subtle invariant worth documenting with a comment.
**Severity:** None (correct, but subtle).
---
### ✅ Verdict
**APPROVED.** The PR is well-structured, solidly tested (22/22 passing), and introduces significant hardening (IPv6, TLS hostname verification, SUID/SGID stripping, protocol bounds, config receive overflow protection, graceful shutdown). The issues found are all low-severity:
- Issue #1 (`EINTR` in `sendfile`) and issue #2 (`strstr("..")` false positives) are worth fixing before merge but are not blockers.
- Issue #3 (bandwidth limiter thread safety) is pre-existing and orthogonal to this PR.
- Issues #4 and #5 are informational only.
sendfile EINTR retry ✅ — if (errno == EINTR) continue; correctly retries the sendfile loop (lines 643-645 of src/shared/file.c).
Path traversal check improved ✅ — has_path_traversal() helper validates .. only as a path component (lines 203-216), replacing naive strstr(".."). Used in file_save_to_disk() at lines 221, 229, 245.
Verdict: APPROVED
## Re-review: ALL FIXES VERIFIED ✅
1. sendfile EINTR retry ✅ — `if (errno == EINTR) continue;` correctly retries the sendfile loop (lines 643-645 of `src/shared/file.c`).
2. Path traversal check improved ✅ — `has_path_traversal()` helper validates `..` only as a path component (lines 203-216), replacing naive `strstr("..")`. Used in `file_save_to_disk()` at lines 221, 229, 245.
**Verdict: APPROVED**
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.
Fixes:\n1. Initialize server->ssl_ctx = NULL in server_create\n2. Update tests for sockaddr_storage API\n3. Update test_file_sendfile.c for file_type indicator
PR Review:
fix/integration-cleanup(shab20e390)Overview
This PR performs a major cleanup and hardening of the transport layer and ancillary modules. It adds IPv4/IPv6 dual-stack support (via
getaddrinfo/sockaddr_storage), asendfile-based zero-copy file transfer path, globstar (**) pattern matching, graceful server shutdown, TLS SNI + hostname verification, exclude/include pattern filtering on the server side, and a comprehensive test suite expansion. 40 files changed, ~1,000 lines added.✅ Positive findings
server_createandclient_connectboth try IPv6 first, fall back to IPv4 viagetaddrinfo+sockaddr_storage.mkdir_rreplacedstrcpywithmemcpyand added bounds checking;str_dupnow usesmemcpy.receive_strenforcesMAX_STRING_SIZE(10 MB);receive_dataenforcesMAX_DATA_SIZE(1 GB).SSL_set_tlsext_host_name) and hostname verification (X509_VERIFY_PARAM_set1_host).SSL_get_verify_resultis checked after handshake.file_save_to_diskblocks absolute symlink targets and..in target; also rejects..indisk_path.file_restore_metadatamasks `S_ISUIDconfig_receivecaps pattern count atMAX_PATTERN_COUNT(10,000) and checks for integer overflow (src/shared/config.c:258-266).server_request_shutdown()+volatile sig_atomic_tflag terminates theacceptloop cleanly onSIGINT/SIGTERM.directory_scanner_create_fulldeep-copies exclude/include patterns and cleans up on allocation failure.data_compressclamps level to[1, 22].data_create_empty(0)malloc(0).test_file_sendfile,test_log,test_multiprocessing,test_transport_tcp,test_transport_ssh,test_transport_tls,test_scanneredge cases). All 22 tests pass.test_file_sendfileandtest_fileskip fork-based tests under valgrind using/proc/self/mapsheuristic.⚠️ Issues found
1.
sendfilemissingEINTRretry (src/shared/file.c:626-631)sendfile(2)can return-1witherrno == EINTRon interruption. This will cause an unnecessary transfer failure. Should retry onEINTR.Severity: Low (rare in practice; signal handler installs are minimal in this codebase).
2. Overly broad path traversal check (
src/shared/file.c:213, 229)Uses
strstrondisk_path. This flags any path containing the substring.., including legitimate names likefoo..bar.txt. While conservative (safe against../), it causes false rejections. A better approach: scan for..as a complete path component (e.g., tokenize by/).Severity: Low (conservative false-positive, not a vulnerability).
3. Bandwidth limiter not thread-safe (
src/shared/protocol.c:15-17)These are not
__thread, so they're shared across threads.bw_tokensis read/written without atomics inbw_throttle(). Pre-existing; not introduced by this PR.Severity: Low-Medium (pre-existing; could cause inaccurate throttling under multithreaded transfers, but not a memory-safety issue).
4.
is_running_under_valgrind()reads at most 4095 bytes (tests/test_utils.h)On a system with an unusually large
/proc/self/maps, thevgpreloadstring could appear after the first 4095 bytes, causing a false negative. In practice,vgpreloadalways appears near the top, so this is unlikely to trigger.Severity: Very Low.
5.
client_connectusesrpafterbreak— but this is safe (src/shared/transport_tcp.c:268)rpis valid because the loop exits viabreakonly whenconnectsucceeded, at which pointrppoints to the currentaddrinfonode. If no connection succeeded, thefd < 0check returns false before this point. No bug — but this is a subtle invariant worth documenting with a comment.Severity: None (correct, but subtle).
✅ Verdict
APPROVED. The PR is well-structured, solidly tested (22/22 passing), and introduces significant hardening (IPv6, TLS hostname verification, SUID/SGID stripping, protocol bounds, config receive overflow protection, graceful shutdown). The issues found are all low-severity:
EINTRinsendfile) and issue #2 (strstr("..")false positives) are worth fixing before merge but are not blockers.Re-review: ALL FIXES VERIFIED ✅
if (errno == EINTR) continue;correctly retries the sendfile loop (lines 643-645 ofsrc/shared/file.c).has_path_traversal()helper validates..only as a path component (lines 203-216), replacing naivestrstr(".."). Used infile_save_to_disk()at lines 221, 229, 245.Verdict: APPROVED
Pull request closed