Fix logic/correctness bugs (#73, #68, #67, #66, #59, #58, #54, #48, #53) #88

Merged

TapTap merged 1 commits from fix/logic-correctness into main 2026-07-21 15:00:48 +02:00

Conversation 2 Commits 1 Files Changed 2

+2 -2

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

Owner

Copy Link

Copy Source

Fixes 9 logic bugs:\n- #73: SSL WANT_WRITE handling\n- #68: Integer overflow bandwidth\n- #67: glob_match ** support\n- #66: g_server signal safety\n- #59: atoi port validation\n- #58: chmod/chown return values\n- #54: Duplicate delta receive\n- #48: compression_level validation\n- #53: log_message const char*

Fixes 9 logic bugs:\n- #73: SSL WANT_WRITE handling\n- #68: Integer overflow bandwidth\n- #67: glob_match ** support\n- #66: g_server signal safety\n- #59: atoi port validation\n- #58: chmod/chown return values\n- #54: Duplicate delta receive\n- #48: compression_level validation\n- #53: log_message const char*

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

Author

Owner

Copy Link

Copy Source

PR #88 Review: fix/logic-correctness (6ada6a5)

Verdict: APPROVE — This is a large, well-structured PR that fixes numerous correctness, security, and compatibility issues. The code quality is high with thorough error handling and comprehensive test coverage. No blocking issues found.

---

Overview

This PR spans ~30 files touching the client CLI, server, scanner, config, protocol, transport (TCP/TLS/SSH), metadata, compression, file I/O, utils (glob), and adds extensive test coverage. The changes fall into these categories:

Category	Changes
Security hardening	Path traversal prevention, SUID/SGID stripping, TLS hostname verification + SNI, fixed-width wire types, input size limits, malloc(0) fix
Protocol correctness	SSL WANT_READ/WANT_WRITE non-blocking handling, bandwidth throttling overflow fix, send_str NULL check, oversized string/data rejection
Memory safety	strcpy→memcpy everywhere, deep-copy patterns in scanner, bounds checking in mkdir_r, malloc(0)→malloc(1), dynamic ssh_argv allocation
Cross-platform / IPv6	sockaddr_storage, getaddrinfo, AF_INET6→AF_INET fallback in server and client
Graceful shutdown	Signal handler no longer calls _exit directly, accept loop checks cleanup flag, EINTR handling
New features	Symlink transfer support, large file streaming (>64MB), --partial resume, --version, globstar (**) patterns, exclude/include pattern propagation to server
Test coverage	7 new test files (sendfile, log, multiprocessing, transport_tcp/ssh/tls), scanner pattern edge cases, oversized string, empty data(0), valgrind detection

---

Issues Found

MEDIUM: test_scanner_max_size — files created before mkdir

File: tests/test_scanner.c

create_test_file(small, "tiny");  // dir doesn't exist yet — fails
create_test_file(large, "...");    // dir doesn't exist yet — fails

mkdir(dir, 0755);
create_test_file(small, "tiny");  // correct call
create_test_file(large, "...");    // correct call

The first pair of create_test_file calls executes before mkdir() — these will fail silently. The files are then created a second time after mkdir. Remove the redundant first pair.

LOW: is_excluded() uses basename() with implementation-defined behavior

File: src/server/server.c:294

char* fname = basename(path_dup);

POSIX basename() (from <libgen.h>) may modify its argument and may return a pointer to internal static storage on some implementations. The code correctly passes a str_dup'd copy, so it is safe here. However, the returned pointer should only be used as read-only (which glob_match does). Consider adding a comment noting that basename result must not be freed.

LOW: glob_match("**/foo", "foo") works via fallback path rather than dedicated ** code

File: src/shared/utils.c

When matching "**/foo" against "foo", the function reaches the literal-mismatch branch and falls into the /***/ fallback (lines 105-111) rather than the dedicated ** code (lines 80-87). The result is correct, but the double-star logic is split across two locations. Consider unifying for clarity.

INFO: config_receive goto-error pattern is fragile

File: src/shared/config.c:339-345

The error: label frees version, send_directory, receive_root_directory, server_host, and config itself — but does not free exclude_patterns or include_patterns. The call sites that do partial allocation explicitly free and NULL-out their arrays before jumping to error. This is correct but fragile — future additions between cleanup and goto could leak.

INFO: file_send_streaming does not send STATUS_ERROR on failure

File: src/shared/file.c:701-739

Not a bug — this is the sender side; connection close upon return serves as the error indicator, matching existing patterns.

---

Highlights

• Security: Path traversal blocked via strstr(..., "..") checks on received paths and symlink targets. SUID/SGID bits stripped on metadata restore. TLS hostname verification with SNI. Fixed-width wire types prevent protocol mismatch between 32/64-bit peers.
• Memory safety: Every strcpy replaced with memcpy. malloc(0) UB fixed. Dynamic ssh_argv with bounds checks. Pattern deep-copy in scanner prevents use-after-free. NULL checks added in metadata_from_buf and config_create.
• Protocol hardening: MAX_STRING_SIZE (10MB) and MAX_DATA_SIZE (1GB) limits prevent OOM from malicious peers. SSL non-blocking retry added to send_n_data/receive_n_data.
• IPv6: Dual-stack support in both server and client via getaddrinfo + sockaddr_storage.
• Globstar: ** pattern matching with tests for prefix, suffix, and mid-path.
• Tests: 7 new test files, extensive scanner pattern edge cases, valgrind-safe fork test skipping, oversized string rejection test.

---

Summary

Severity	Count
Critical	0
High	0
Medium	1
Low	2
Info	2

This PR significantly improves the correctness, security, and cross-platform compatibility of FastSync. The code is well-structured, errors are properly handled, and the test coverage expansion is excellent. Approved.

## PR #88 Review: `fix/logic-correctness` (6ada6a5) **Verdict: APPROVE** — This is a large, well-structured PR that fixes numerous correctness, security, and compatibility issues. The code quality is high with thorough error handling and comprehensive test coverage. No blocking issues found. --- ### Overview This PR spans ~30 files touching the client CLI, server, scanner, config, protocol, transport (TCP/TLS/SSH), metadata, compression, file I/O, utils (glob), and adds extensive test coverage. The changes fall into these categories: | Category | Changes | |---|---| | **Security hardening** | Path traversal prevention, SUID/SGID stripping, TLS hostname verification + SNI, fixed-width wire types, input size limits, malloc(0) fix | | **Protocol correctness** | SSL WANT_READ/WANT_WRITE non-blocking handling, bandwidth throttling overflow fix, send_str NULL check, oversized string/data rejection | | **Memory safety** | strcpy→memcpy everywhere, deep-copy patterns in scanner, bounds checking in mkdir_r, malloc(0)→malloc(1), dynamic ssh_argv allocation | | **Cross-platform / IPv6** | sockaddr_storage, getaddrinfo, AF_INET6→AF_INET fallback in server and client | | **Graceful shutdown** | Signal handler no longer calls _exit directly, accept loop checks cleanup flag, EINTR handling | | **New features** | Symlink transfer support, large file streaming (>64MB), --partial resume, --version, globstar (**) patterns, exclude/include pattern propagation to server | | **Test coverage** | 7 new test files (sendfile, log, multiprocessing, transport_tcp/ssh/tls), scanner pattern edge cases, oversized string, empty data(0), valgrind detection | --- ### Issues Found #### MEDIUM: `test_scanner_max_size` — files created before mkdir **File:** `tests/test_scanner.c` ```c create_test_file(small, "tiny"); // dir doesn't exist yet — fails create_test_file(large, "..."); // dir doesn't exist yet — fails mkdir(dir, 0755); create_test_file(small, "tiny"); // correct call create_test_file(large, "..."); // correct call ``` The first pair of `create_test_file` calls executes before `mkdir()` — these will fail silently. The files are then created a second time after mkdir. Remove the redundant first pair. #### LOW: `is_excluded()` uses `basename()` with implementation-defined behavior **File:** `src/server/server.c:294` ```c char* fname = basename(path_dup); ``` POSIX `basename()` (from `<libgen.h>`) may modify its argument and may return a pointer to internal static storage on some implementations. The code correctly passes a `str_dup`'d copy, so it is safe here. However, the returned pointer should only be used as read-only (which `glob_match` does). Consider adding a comment noting that `basename` result must not be freed. #### LOW: `glob_match("**/foo", "foo")` works via fallback path rather than dedicated `**` code **File:** `src/shared/utils.c` When matching `"**/foo"` against `"foo"`, the function reaches the literal-mismatch branch and falls into the `/***/` fallback (lines 105-111) rather than the dedicated `**` code (lines 80-87). The result is correct, but the double-star logic is split across two locations. Consider unifying for clarity. #### INFO: `config_receive` goto-error pattern is fragile **File:** `src/shared/config.c:339-345` The `error:` label frees `version`, `send_directory`, `receive_root_directory`, `server_host`, and `config` itself — but does not free `exclude_patterns` or `include_patterns`. The call sites that do partial allocation explicitly free and NULL-out their arrays before jumping to `error`. This is correct but fragile — future additions between cleanup and goto could leak. #### INFO: `file_send_streaming` does not send STATUS_ERROR on failure **File:** `src/shared/file.c:701-739` Not a bug — this is the sender side; connection close upon return serves as the error indicator, matching existing patterns. --- ### Highlights - **Security**: Path traversal blocked via `strstr(..., "..")` checks on received paths and symlink targets. SUID/SGID bits stripped on metadata restore. TLS hostname verification with SNI. Fixed-width wire types prevent protocol mismatch between 32/64-bit peers. - **Memory safety**: Every `strcpy` replaced with `memcpy`. `malloc(0)` UB fixed. Dynamic ssh_argv with bounds checks. Pattern deep-copy in scanner prevents use-after-free. NULL checks added in `metadata_from_buf` and `config_create`. - **Protocol hardening**: `MAX_STRING_SIZE` (10MB) and `MAX_DATA_SIZE` (1GB) limits prevent OOM from malicious peers. SSL non-blocking retry added to `send_n_data`/`receive_n_data`. - **IPv6**: Dual-stack support in both server and client via `getaddrinfo` + `sockaddr_storage`. - **Globstar**: `**` pattern matching with tests for prefix, suffix, and mid-path. - **Tests**: 7 new test files, extensive scanner pattern edge cases, valgrind-safe fork test skipping, oversized string rejection test. --- ### Summary | Severity | Count | |---|---| | Critical | 0 | | High | 0 | | Medium | 1 | | Low | 2 | | Info | 2 | This PR significantly improves the correctness, security, and cross-platform compatibility of FastSync. The code is well-structured, errors are properly handled, and the test coverage expansion is excellent. **Approved.**

TapTap added 14 commits 2026-07-21 14:20:12 +02:00

fix: adapt tests and fix bugs from enhancements rebase onto main ... 7ecba4e0d5

- 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

ci: trigger CI on PR 9e3e0f57a0

fix: review fixes — getsockname, test assertion, memcpy, clang-format 3923421224

ci: re-trigger after review fixes 266369b1f2

ci: re-trigger after fixes a91267ca1b

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

ci: re-trigger after cppcheck fixes 70e1c6788a

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

fix: security issues — path traversal, TLS hostname, SUID, OOM, stack overflow 224e7e8599

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

fix: test quality — cppcheck suppressions, TLS test addresses, log test isolation 5d819f7388

fix: clang-format compliance cdd6d1cfed

fix: address review findings — localtime_r, test_scanner cleanup 0cbc873f5c

fix: address review findings — test_scanner cleanup, basename comment f932a48910

TapTap force-pushed fix/logic-correctness from 6ada6a5aa7 to f932a48910 2026-07-21 14:20:12 +02:00 Compare

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

Author

Owner

Copy Link

Copy Source

Re-review: ALL FIXES VERIFIED ✅

1. test_scanner_max_size cleanup ✅
2. basename() comment added ✅

Verdict: APPROVED

## Re-review: ALL FIXES VERIFIED ✅ 1. test_scanner_max_size cleanup ✅ 2. basename() comment added ✅ **Verdict: APPROVED**

TapTap merged commit 1c3b752a8e into main 2026-07-21 15:00:48 +02:00

TapTap deleted branch fix/logic-correctness 2026-07-21 15:00:54 +02:00

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