Fixes 3 issues:\n- #61: data.h include quotes → angle brackets\n- #51: FILE_METADATA_WIRE_SIZE portable (fixed-width types)\n- #52: Add --version flag to client and server
Fixes 3 issues:\n- #61: data.h include quotes → angle brackets\n- #51: FILE_METADATA_WIRE_SIZE portable (fixed-width types)\n- #52: Add --version flag to client and server
PR:#92 (fix/refactoring @ 5525611) Scope: 30+ files changed across src/ and tests/ Issues addressed:#61 (IPv6), #51 (cross-platform metadata), #52 (symlinks), plus protocol hardening, streaming, graceful shutdown, TLS improvements, globstar, test coverage
VERDICT: APPROVE with minor observations
This is a well-structured, comprehensive refactoring. The code quality is significantly improved over main. No critical or high-severity issues found. The changes are logically sound and the test coverage expansion is commendable.
BUILD & TESTS
The PR passed CI (lint + build + run tests + address sanitizer)
Fork-based tests correctly skip under valgrind via is_running_under_valgrind()
CATEGORY: Memory Safety
strcpy -> memcpy migration throughout ✅
All strcpy calls in file.c, utils.c, data.c replaced with memcpy. Eliminates buffer over-read from null-terminator search.
malloc(0) UB fix in data_create_empty✅
src/shared/data.c:620-623 — allocates at least 1 byte when data_size == 0.
Compression level clamping ✅
compression.c:419-427 — clamps [1, 22] for zstd before use.
realloc safe pattern ✅
client_cli.c:102-118 — uses temp pointer before assignment, correct on OOM.
receive_str size limit ✅
MAX_STRING_SIZE (10 MB) prevents huge allocations from malformed input.
receive_data size limit ✅
MAX_DATA_SIZE (1 GB) prevents allocation bomb.
CATEGORY: Thread Safety
localtime() not thread-safe ⚠️MEDIUM
src/shared/log.c:17 — localtime(&now) uses static internal storage. In multithreaded mode (pipeline sender/receiver threads both call log_message), this races. Replace with localtime_r(&now, &result_buf).
TLS variables correctly used ✅
io_read_fd, io_write_fd, io_ssl are static __thread in protocol.c — each thread has its own copy. Good.
CATEGORY: Protocol Safety
Metadata wire format portability ✅Excellent
metadata.c — switched mode_t/uid_t/gid_t/time_t/long to int32_t/int64_t. Wire format is now platform-independent. This was issue #51. static_assert or _Static_assert would be a nice addition for FILE_METADATA_WIRE_SIZE.
SSL WANT_READ/WANT_WRITE handling ✅ (with note)
protocol.c:80-82, 114-116 — SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITE now retry via continue. However, if the underlying socket is non-blocking (which could happen in future), this creates a busy-loop with no poll()/select(). Currently sockets are blocking with SO_RCVTIMEO/SO_SNDTIMEO, so this doesn't fire in practice. If non-blocking mode is ever introduced, these loops need poll() before retry.
NULL pointer guard in send_str✅
protocol.c:153 — added NULL check for data parameter. Prevents crash.
CATEGORY: Security
Path traversal protection in symlink handling ✅
file.c:794-805 — blocks symlink targets starting with / or containing ... Also validates the concatenated disk_path. This is defense-in-depth and appropriate.
strstr(..., "..") is a blunt instrument ⚠️LOW
file.c:794, 802, 918 — using strstr(full_path, "..") has false positives on legitimate filenames like foo..txt or a..b. However, this is defense-in-depth (the .. must first come from the remote end over the wire, which only happens during incremental/delta). Acceptable for current purposes, but realpath()/canonicalization would be more precise.
TLS SNI and hostname verification ✅Excellent
transport_tls.c:168-172 — SSL_set_tlsext_host_name() sets SNI, and X509_VERIFY_PARAM_set1_host() enables hostname matching. This was missing before.
TLS non-verification warning ✅
transport_tls.c:99-103 — when no CA is provided for the client, logs a warning and sets SSL_VERIFY_NONE. Explicit is better than implicit.
server.c:291 — if str_dup(path) returns NULL, is_excluded returns false, meaning the file is NOT excluded. Desirable: if OOM, the safe behavior is to process the file (not silently skip it). However, this could be a surprise. Worth a comment.
file_content_to_buffer fragile without NULL data check ⚠️LOW
file.c:547 — the function assumes file->data->data is non-NULL. Its only caller (file_load_data) always allocates before calling, but the function itself doesn't guard against NULL data. Minor defensive gap.
mkdir_r buffer overflow protection ✅
utils.c:39-42 — correctly checks pos + part_len + 1 >= buf_size before memcpy. Prevents buffer overflow from deeply nested paths.
Graceful shutdown fix ✅Excellent
server.c:359-367 — signal handler now sets a flag instead of calling _exit(0). The accept loop breaks on EINTR, main returns and frees the server. Previously, _exit(0) prevented clean shutdown.
CATEGORY: Code Quality / Design
DRY improvements ✅
receive_and_decompress() and receive_and_assign_metadata() helpers extracted in file.c, reducing duplication in receive_incremental_check, receive_delta_file, and file_receive.
IPv6 dual-stack ✅
transport_tcp.c — tries AF_INET6 first with fallback to AF_INET at socket, bind, and connect levels. Uses getaddrinfo() for client connects. Good modern networking.
File type indicator added to protocol ✅
Sending file->type as int before data lets the receiver distinguish FILE_TYPE_REGULAR, FILE_TYPE_SYMLINK, FILE_TYPE_DIR. Wire format is backward-compatible? Actually, this changes the protocol for all transfers — older servers would interpret the file type int as the first byte of the data size. This is a breaking protocol change. The version was bumped to 1.3.0 in config.h, so clients/servers on different versions will reject handshake. This is the correct approach.
Streaming mode for large files ✅
file_send_streaming() — files >64 MB are sent in 1 MB chunks without loading into RAM. Avoids OOM on huge files.
CATEGORY: Test Quality
New tests are thorough and follow existing patterns:
localtime() not thread-safe -> use localtime_r(); SSL WANT_* busy-loop potential
LOW
4
strstr false positives on ..; is_excluded OOM pass-through; file_content_to_buffer NULL data guard; minor SSH _exit leak (intentional)
Final verdict: APPROVE. This is a high-quality refactoring PR that addresses multiple longstanding issues, hardens the protocol, adds IPv6 and symlink support, and significantly expands the test suite. The remaining issues are minor and can be addressed in follow-up PRs.
## COMBINED REVIEW REPORT
**PR:** #92 (`fix/refactoring` @ `5525611`)
**Scope:** 30+ files changed across src/ and tests/
**Issues addressed:** #61 (IPv6), #51 (cross-platform metadata), #52 (symlinks), plus protocol hardening, streaming, graceful shutdown, TLS improvements, globstar, test coverage
---
## VERDICT: APPROVE with minor observations
This is a well-structured, comprehensive refactoring. The code quality is significantly improved over main. No critical or high-severity issues found. The changes are logically sound and the test coverage expansion is commendable.
---
## BUILD & TESTS
- The PR passed CI (lint + build + run tests + address sanitizer)
- New tests cover: sendfile, multiprocessing, log, transport_tcp/ssh/tls, globstar, scanner edge cases
- Fork-based tests correctly skip under valgrind via `is_running_under_valgrind()`
---
## CATEGORY: Memory Safety
### `strcpy` -> `memcpy` migration throughout :white_check_mark:
All `strcpy` calls in `file.c`, `utils.c`, `data.c` replaced with `memcpy`. Eliminates buffer over-read from null-terminator search.
### `malloc(0)` UB fix in `data_create_empty` :white_check_mark:
`src/shared/data.c:620-623` — allocates at least 1 byte when `data_size == 0`.
### Compression level clamping :white_check_mark:
`compression.c:419-427` — clamps [1, 22] for zstd before use.
### `realloc` safe pattern :white_check_mark:
`client_cli.c:102-118` — uses temp pointer before assignment, correct on OOM.
### `receive_str` size limit :white_check_mark:
`MAX_STRING_SIZE` (10 MB) prevents huge allocations from malformed input.
### `receive_data` size limit :white_check_mark:
`MAX_DATA_SIZE` (1 GB) prevents allocation bomb.
---
## CATEGORY: Thread Safety
### `localtime()` not thread-safe :warning: **MEDIUM**
`src/shared/log.c:17` — `localtime(&now)` uses static internal storage. In multithreaded mode (pipeline sender/receiver threads both call `log_message`), this races. Replace with `localtime_r(&now, &result_buf)`.
### TLS variables correctly used :white_check_mark:
`io_read_fd`, `io_write_fd`, `io_ssl` are `static __thread` in `protocol.c` — each thread has its own copy. Good.
---
## CATEGORY: Protocol Safety
### Metadata wire format portability :white_check_mark: **Excellent**
`metadata.c` — switched `mode_t`/`uid_t`/`gid_t`/`time_t`/`long` to `int32_t`/`int64_t`. Wire format is now platform-independent. This was issue #51. `static_assert` or `_Static_assert` would be a nice addition for `FILE_METADATA_WIRE_SIZE`.
### SSL WANT_READ/WANT_WRITE handling :white_check_mark: (with note)
`protocol.c:80-82, 114-116` — `SSL_ERROR_WANT_READ`/`SSL_ERROR_WANT_WRITE` now retry via `continue`. However, if the underlying socket is non-blocking (which could happen in future), this creates a busy-loop with no `poll()`/`select()`. Currently sockets are blocking with `SO_RCVTIMEO`/`SO_SNDTIMEO`, so this doesn't fire in practice. If non-blocking mode is ever introduced, these loops need `poll()` before retry.
### NULL pointer guard in `send_str` :white_check_mark:
`protocol.c:153` — added NULL check for `data` parameter. Prevents crash.
---
## CATEGORY: Security
### Path traversal protection in symlink handling :white_check_mark:
`file.c:794-805` — blocks symlink targets starting with `/` or containing `..`. Also validates the concatenated disk_path. This is defense-in-depth and appropriate.
### `strstr(..., "..")` is a blunt instrument :warning: **LOW**
`file.c:794, 802, 918` — using `strstr(full_path, "..")` has false positives on legitimate filenames like `foo..txt` or `a..b`. However, this is defense-in-depth (the `..` must first come from the remote end over the wire, which only happens during incremental/delta). Acceptable for current purposes, but realpath()/canonicalization would be more precise.
### TLS SNI and hostname verification :white_check_mark: **Excellent**
`transport_tls.c:168-172` — `SSL_set_tlsext_host_name()` sets SNI, and `X509_VERIFY_PARAM_set1_host()` enables hostname matching. This was missing before.
### TLS non-verification warning :white_check_mark:
`transport_tls.c:99-103` — when no CA is provided for the client, logs a warning and sets `SSL_VERIFY_NONE`. Explicit is better than implicit.
### chmod strips SUID/SGID :white_check_mark:
`metadata.c:153` — `chmod(path, metadata->mode & 07777 & ~(S_ISUID | S_ISGID))`. Good security hygiene.
---
## CATEGORY: Logic / Correctness
### `is_excluded` behavior on OOM :warning: **LOW**
`server.c:291` — if `str_dup(path)` returns NULL, `is_excluded` returns `false`, meaning the file is NOT excluded. Desirable: if OOM, the safe behavior is to process the file (not silently skip it). However, this could be a surprise. Worth a comment.
### Partial file path buffer correctly sized :white_check_mark:
`file.c:925-932` — `malloc(strlen(full_path) + 20)` safely holds `full_path` + `.fastsync-partial` (17 chars + null). No truncation risk.
### `file_content_to_buffer` fragile without NULL data check :warning: **LOW**
`file.c:547` — the function assumes `file->data->data` is non-NULL. Its only caller (`file_load_data`) always allocates before calling, but the function itself doesn't guard against NULL data. Minor defensive gap.
### `mkdir_r` buffer overflow protection :white_check_mark:
`utils.c:39-42` — correctly checks `pos + part_len + 1 >= buf_size` before `memcpy`. Prevents buffer overflow from deeply nested paths.
### Graceful shutdown fix :white_check_mark: **Excellent**
`server.c:359-367` — signal handler now sets a flag instead of calling `_exit(0)`. The accept loop breaks on `EINTR`, main returns and frees the server. Previously, `_exit(0)` prevented clean shutdown.
---
## CATEGORY: Code Quality / Design
### DRY improvements :white_check_mark:
- `receive_and_decompress()` and `receive_and_assign_metadata()` helpers extracted in `file.c`, reducing duplication in `receive_incremental_check`, `receive_delta_file`, and `file_receive`.
### IPv6 dual-stack :white_check_mark:
`transport_tcp.c` — tries `AF_INET6` first with fallback to `AF_INET` at socket, bind, and connect levels. Uses `getaddrinfo()` for client connects. Good modern networking.
### File type indicator added to protocol :white_check_mark:
Sending `file->type` as `int` before data lets the receiver distinguish `FILE_TYPE_REGULAR`, `FILE_TYPE_SYMLINK`, `FILE_TYPE_DIR`. Wire format is backward-compatible? Actually, this changes the protocol for all transfers — older servers would interpret the file type int as the first byte of the data size. **This is a breaking protocol change.** The version was bumped to `1.3.0` in `config.h`, so clients/servers on different versions will reject handshake. This is the correct approach.
### Streaming mode for large files :white_check_mark:
`file_send_streaming()` — files >64 MB are sent in 1 MB chunks without loading into RAM. Avoids OOM on huge files.
---
## CATEGORY: Test Quality
New tests are thorough and follow existing patterns:
- `test_file_sendfile` — basic, empty, missing, compression fallback, no-path modes
- `test_multiprocessing` — sender/receiver context creation/destroy with varying caps
- `test_log` — log level filtering, format string safety (crash regression tests)
- `test_transport_tcp` — client/server create/delete, disconnect, null handling
- `test_transport_ssh` — invalid/malformed/unreachable destinations
- `test_transport_tls` — init, bad cert, null paths, server bad cert
- `test_glob` — 4 new tests for globstar (`**`) patterns
- `test_scanner` — 8 new tests for exclude/include patterns and size filters
- `test_protocol` — `receive_str_oversized` validates `MAX_STRING_SIZE` enforcement
- `test_data` — `data_create_empty_zero` validates `malloc(0)` fix
---
## SUMMARY OF ISSUES
| Severity | Count | Key Items |
|----------|-------|-----------|
| CRITICAL | 0 | — |
| HIGH | 0 | — |
| MEDIUM | 2 | `localtime()` not thread-safe -> use `localtime_r()`; SSL WANT_* busy-loop potential |
| LOW | 4 | `strstr` false positives on `..`; `is_excluded` OOM pass-through; `file_content_to_buffer` NULL data guard; minor SSH `_exit` leak (intentional) |
**Final verdict:** APPROVE. This is a high-quality refactoring PR that addresses multiple longstanding issues, hardens the protocol, adds IPv6 and symlink support, and significantly expands the test suite. The remaining issues are minor and can be addressed in follow-up PRs.
localtime_r in log.c ✅ — struct tm result_buf on stack, localtime_r(&now, &result_buf) at line 17-18
test_scanner_max_size cleanup ✅ — function does not exist on this branch; all create_test_file calls properly follow mkdir
Verdict: APPROVED
## Re-review: ALL FIXES VERIFIED ✅
1. localtime_r in log.c ✅ — `struct tm result_buf` on stack, `localtime_r(&now, &result_buf)` at line 17-18
2. test_scanner_max_size cleanup ✅ — function does not exist on this branch; all `create_test_file` calls properly follow `mkdir`
**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 3 issues:\n- #61: data.h include quotes → angle brackets\n- #51: FILE_METADATA_WIRE_SIZE portable (fixed-width types)\n- #52: Add --version flag to client and server
COMBINED REVIEW REPORT
PR: #92 (
fix/refactoring@5525611)Scope: 30+ files changed across src/ and tests/
Issues addressed: #61 (IPv6), #51 (cross-platform metadata), #52 (symlinks), plus protocol hardening, streaming, graceful shutdown, TLS improvements, globstar, test coverage
VERDICT: APPROVE with minor observations
This is a well-structured, comprehensive refactoring. The code quality is significantly improved over main. No critical or high-severity issues found. The changes are logically sound and the test coverage expansion is commendable.
BUILD & TESTS
is_running_under_valgrind()CATEGORY: Memory Safety
strcpy->memcpymigration throughout ✅All
strcpycalls infile.c,utils.c,data.creplaced withmemcpy. Eliminates buffer over-read from null-terminator search.malloc(0)UB fix indata_create_empty✅src/shared/data.c:620-623— allocates at least 1 byte whendata_size == 0.Compression level clamping ✅
compression.c:419-427— clamps [1, 22] for zstd before use.reallocsafe pattern ✅client_cli.c:102-118— uses temp pointer before assignment, correct on OOM.receive_strsize limit ✅MAX_STRING_SIZE(10 MB) prevents huge allocations from malformed input.receive_datasize limit ✅MAX_DATA_SIZE(1 GB) prevents allocation bomb.CATEGORY: Thread Safety
localtime()not thread-safe ⚠️ MEDIUMsrc/shared/log.c:17—localtime(&now)uses static internal storage. In multithreaded mode (pipeline sender/receiver threads both calllog_message), this races. Replace withlocaltime_r(&now, &result_buf).TLS variables correctly used ✅
io_read_fd,io_write_fd,io_sslarestatic __threadinprotocol.c— each thread has its own copy. Good.CATEGORY: Protocol Safety
Metadata wire format portability ✅ Excellent
metadata.c— switchedmode_t/uid_t/gid_t/time_t/longtoint32_t/int64_t. Wire format is now platform-independent. This was issue #51.static_assertor_Static_assertwould be a nice addition forFILE_METADATA_WIRE_SIZE.SSL WANT_READ/WANT_WRITE handling ✅ (with note)
protocol.c:80-82, 114-116—SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITEnow retry viacontinue. However, if the underlying socket is non-blocking (which could happen in future), this creates a busy-loop with nopoll()/select(). Currently sockets are blocking withSO_RCVTIMEO/SO_SNDTIMEO, so this doesn't fire in practice. If non-blocking mode is ever introduced, these loops needpoll()before retry.NULL pointer guard in
send_str✅protocol.c:153— added NULL check fordataparameter. Prevents crash.CATEGORY: Security
Path traversal protection in symlink handling ✅
file.c:794-805— blocks symlink targets starting with/or containing... Also validates the concatenated disk_path. This is defense-in-depth and appropriate.strstr(..., "..")is a blunt instrument ⚠️ LOWfile.c:794, 802, 918— usingstrstr(full_path, "..")has false positives on legitimate filenames likefoo..txtora..b. However, this is defense-in-depth (the..must first come from the remote end over the wire, which only happens during incremental/delta). Acceptable for current purposes, but realpath()/canonicalization would be more precise.TLS SNI and hostname verification ✅ Excellent
transport_tls.c:168-172—SSL_set_tlsext_host_name()sets SNI, andX509_VERIFY_PARAM_set1_host()enables hostname matching. This was missing before.TLS non-verification warning ✅
transport_tls.c:99-103— when no CA is provided for the client, logs a warning and setsSSL_VERIFY_NONE. Explicit is better than implicit.chmod strips SUID/SGID ✅
metadata.c:153—chmod(path, metadata->mode & 07777 & ~(S_ISUID | S_ISGID)). Good security hygiene.CATEGORY: Logic / Correctness
is_excludedbehavior on OOM ⚠️ LOWserver.c:291— ifstr_dup(path)returns NULL,is_excludedreturnsfalse, meaning the file is NOT excluded. Desirable: if OOM, the safe behavior is to process the file (not silently skip it). However, this could be a surprise. Worth a comment.Partial file path buffer correctly sized ✅
file.c:925-932—malloc(strlen(full_path) + 20)safely holdsfull_path+.fastsync-partial(17 chars + null). No truncation risk.file_content_to_bufferfragile without NULL data check ⚠️ LOWfile.c:547— the function assumesfile->data->datais non-NULL. Its only caller (file_load_data) always allocates before calling, but the function itself doesn't guard against NULL data. Minor defensive gap.mkdir_rbuffer overflow protection ✅utils.c:39-42— correctly checkspos + part_len + 1 >= buf_sizebeforememcpy. Prevents buffer overflow from deeply nested paths.Graceful shutdown fix ✅ Excellent
server.c:359-367— signal handler now sets a flag instead of calling_exit(0). The accept loop breaks onEINTR, main returns and frees the server. Previously,_exit(0)prevented clean shutdown.CATEGORY: Code Quality / Design
DRY improvements ✅
receive_and_decompress()andreceive_and_assign_metadata()helpers extracted infile.c, reducing duplication inreceive_incremental_check,receive_delta_file, andfile_receive.IPv6 dual-stack ✅
transport_tcp.c— triesAF_INET6first with fallback toAF_INETat socket, bind, and connect levels. Usesgetaddrinfo()for client connects. Good modern networking.File type indicator added to protocol ✅
Sending
file->typeasintbefore data lets the receiver distinguishFILE_TYPE_REGULAR,FILE_TYPE_SYMLINK,FILE_TYPE_DIR. Wire format is backward-compatible? Actually, this changes the protocol for all transfers — older servers would interpret the file type int as the first byte of the data size. This is a breaking protocol change. The version was bumped to1.3.0inconfig.h, so clients/servers on different versions will reject handshake. This is the correct approach.Streaming mode for large files ✅
file_send_streaming()— files >64 MB are sent in 1 MB chunks without loading into RAM. Avoids OOM on huge files.CATEGORY: Test Quality
New tests are thorough and follow existing patterns:
test_file_sendfile— basic, empty, missing, compression fallback, no-path modestest_multiprocessing— sender/receiver context creation/destroy with varying capstest_log— log level filtering, format string safety (crash regression tests)test_transport_tcp— client/server create/delete, disconnect, null handlingtest_transport_ssh— invalid/malformed/unreachable destinationstest_transport_tls— init, bad cert, null paths, server bad certtest_glob— 4 new tests for globstar (**) patternstest_scanner— 8 new tests for exclude/include patterns and size filterstest_protocol—receive_str_oversizedvalidatesMAX_STRING_SIZEenforcementtest_data—data_create_empty_zerovalidatesmalloc(0)fixSUMMARY OF ISSUES
localtime()not thread-safe -> uselocaltime_r(); SSL WANT_* busy-loop potentialstrstrfalse positives on..;is_excludedOOM pass-through;file_content_to_bufferNULL data guard; minor SSH_exitleak (intentional)Final verdict: APPROVE. This is a high-quality refactoring PR that addresses multiple longstanding issues, hardens the protocol, adds IPv6 and symlink support, and significantly expands the test suite. The remaining issues are minor and can be addressed in follow-up PRs.
Re-review: ALL FIXES VERIFIED ✅
struct tm result_bufon stack,localtime_r(&now, &result_buf)at line 17-18create_test_filecalls properly followmkdirVerdict: APPROVED
Pull request closed