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:
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.**
- 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
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 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*
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:
Issues Found
MEDIUM:
test_scanner_max_size— files created before mkdirFile:
tests/test_scanner.cThe first pair of
create_test_filecalls executes beforemkdir()— these will fail silently. The files are then created a second time after mkdir. Remove the redundant first pair.LOW:
is_excluded()usesbasename()with implementation-defined behaviorFile:
src/server/server.c:294POSIX
basename()(from<libgen.h>) may modify its argument and may return a pointer to internal static storage on some implementations. The code correctly passes astr_dup'd copy, so it is safe here. However, the returned pointer should only be used as read-only (whichglob_matchdoes). Consider adding a comment noting thatbasenameresult must not be freed.LOW:
glob_match("**/foo", "foo")works via fallback path rather than dedicated**codeFile:
src/shared/utils.cWhen 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_receivegoto-error pattern is fragileFile:
src/shared/config.c:339-345The
error:label freesversion,send_directory,receive_root_directory,server_host, andconfigitself — but does not freeexclude_patternsorinclude_patterns. The call sites that do partial allocation explicitly free and NULL-out their arrays before jumping toerror. This is correct but fragile — future additions between cleanup and goto could leak.INFO:
file_send_streamingdoes not send STATUS_ERROR on failureFile:
src/shared/file.c:701-739Not a bug — this is the sender side; connection close upon return serves as the error indicator, matching existing patterns.
Highlights
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.strcpyreplaced withmemcpy.malloc(0)UB fixed. Dynamic ssh_argv with bounds checks. Pattern deep-copy in scanner prevents use-after-free. NULL checks added inmetadata_from_bufandconfig_create.MAX_STRING_SIZE(10MB) andMAX_DATA_SIZE(1GB) limits prevent OOM from malicious peers. SSL non-blocking retry added tosend_n_data/receive_n_data.getaddrinfo+sockaddr_storage.**pattern matching with tests for prefix, suffix, and mid-path.Summary
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.
6ada6a5aa7tof932a48910Re-review: ALL FIXES VERIFIED ✅
Verdict: APPROVED