Fix integration issues: ssl_ctx init + test API updates #89

Closed

TapTap wants to merge 13 commits from fix/integration-cleanup into main

pull from: fix/integration-cleanup

merge into: TapTap:main

TapTap:dev

TapTap:evaluation-of-codebase-by-human

TapTap:integration/all-fixes-v2

TapTap:fix/features-v2

TapTap:fix/bugs-v2

TapTap:fix/quality-v2

TapTap:fix/security-v2

TapTap:main

TapTap:integration/all-fixes

TapTap:fix/test-skip-tls

TapTap:fix/rsync-parity-all

TapTap:fix/test-coverage-all

TapTap:fix/refactoring-all

TapTap:fix/bugs-all

TapTap:fix/security-all

TapTap:integration/2026-07-29-batch-fix

TapTap:fix/refactor-cli-config

TapTap:fix/null-checks-cli

TapTap:merge-all-v2

TapTap:revert-pr-143-merge

TapTap:fix/protocol-improvements

TapTap:fix/cli-features

TapTap:fix/tests-and-docs

TapTap:fix/security-hardening

TapTap:fix/performance

TapTap:fix/memory-safety

TapTap:fix/test-coverage

TapTap:fix/refactoring

TapTap:ci/tier1-review-automation

TapTap:tls-transport

TapTap:add-gh-dependency

TapTap:add-readme

Conversation 2 Commits 13 Files Changed 41

+2130 -180

TapTap commented 2026-07-20 19:39:54 +02:00

Owner

Copy Link

Copy Source

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

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

TapTap added 1 commit 2026-07-20 19:39:55 +02:00

fix: initialize ssl_ctx in server_create, update tests for sockaddr_storage API a2bcda1fcf

TapTap added 1 commit 2026-07-20 19:48:36 +02:00

ci: trigger CI on PR 81739b79c5

TapTap added 1 commit 2026-07-20 19:52:55 +02:00

fix: review fixes — getsockname, memcpy, clang-format 0d57f3c812

TapTap added 1 commit 2026-07-20 20:11:29 +02:00

ci: re-trigger after review fixes c5d8a4e2ea

TapTap added 1 commit 2026-07-20 20:28:22 +02:00

ci: re-trigger after fixes b816437777

TapTap added 1 commit 2026-07-20 20:39:43 +02:00

fix: cppcheck — remove redundant free before _exit in transport_ssh.c 2265f93a18

TapTap added 1 commit 2026-07-20 20:41:23 +02:00

ci: re-trigger after cppcheck fixes 432e9a3ed4

TapTap added 1 commit 2026-07-20 21:09:13 +02:00

fix: accept_loop race condition, auto-detect valgrind, cppcheck suppressions eb6034044b

TapTap added 1 commit 2026-07-20 21:26:03 +02:00

fix: memory safety — malloc NULL checks, strcpy→memcpy, str_dup NULL check, remove unused include 88ad77e397

TapTap added 1 commit 2026-07-20 21:26:14 +02:00

fix: security issues — path traversal, TLS hostname, SUID, OOM, stack overflow 5b5c7b2a7e

TapTap added 1 commit 2026-07-20 21:27:38 +02:00

fix: test quality — cppcheck suppressions, TLS test addresses, log test isolation 58a83193ff

TapTap added 1 commit 2026-07-20 22:01:01 +02:00

fix: clang-format compliance b20e39012e

TapTap commented 2026-07-20 22:15:34 +02:00

Author

Owner

Copy Link

Copy Source

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
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)

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)

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)

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)

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)

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.

## 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.

TapTap added 1 commit 2026-07-21 14:17:53 +02:00

fix: address review findings — sendfile EINTR, path traversal check bd2af8071a

TapTap commented 2026-07-21 14:34:48 +02:00

Author

Owner

Copy Link

Copy Source

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

## 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**

TapTap referenced this issue from a commit 2026-07-21 15:11:24 +02:00

Merge all remaining PRs (#91, #92, #87, #89) into single branch

TapTap referenced this pull request 2026-07-21 15:11:28 +02:00

Merge remaining 4 PRs: memory safety, refactoring, test coverage, integration cleanup #93

TapTap closed this pull request 2026-07-21 16:10:18 +02:00

Some checks are pending

Hide all checks

CI / lint (pull_request) Successful in 9s

Details

CI / sanitizers (address) (pull_request) Successful in 16s

Details

CI / sanitizers (undefined) (pull_request) Successful in 16s

Details

CI / coverage (pull_request) Successful in 11s

Details

CI / fuzz-build (pull_request) Successful in 13s

Details

CI / valgrind (pull_request) Successful in 12s

Details

CI / build-and-test (pull_request) Successful in 55s

Details

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Defect or bug

enhancement

Feature or improvement

memory

Memory safety issue

needs-triage

Needs human review

performance

Performance issue

quality

Code quality improvement

security

Security vulnerability

threading

Threading or concurrency

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

TapTap

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TapTap/FastSync#89