Use-after-free on partial thread creation failure. If thrd_create(&receiver,...) succeeds but thrd_create(&writer,...) fails, the receiver thread is NOT joined before pipeline_context_receiver_destroy(context) is called. The receiver thread continues accessing freed mutexes, queues, and config. Fix: Join the receiver thread before destroying the context on writer creation failure.
2
client_send.c:529
memory
NULL dereference in manifest creation.str_dup(p) can return NULL on allocation failure. array_list_add(manifest, NULL) adds NULL to the list. Later, send_delete_manifest() calls send_str(fd, (char*)manifest->items[i]) which calls strlen(NULL) — segfault. Fix: Check str_dup() return value before array_list_add; skip or abort on NULL.
3
client_send.c:388
memory
Same NULL dereference in multithreaded path.array_list_add(context->manifest, str_dup(p)) can add NULL to the manifest list, causing the same crash downstream. Fix: Same as above — check str_dup() return value.
WARNING (5)
#
File:Line
Category
Issue
4
protocol.c:18
thread
__thread io_ssl contradicts documented guidance. AGENTS.md explicitly warns: "io_ssl must NOT be thread-local". While the current code explicitly calls io_set_ssl() in each thread (making __thread safe), this contradicts documented guidance and makes the code fragile. Fix: Update AGENTS.md to document the new per-thread io_set_ssl() contract.
5
client_cli.c:263-269
memory
FILE leak on multiple --log-file arguments.* If --log-file is specified twice, the previous FILE* in config->log_file is overwritten without fclose(). Fix: Add if (config->log_file) fclose(config->log_file); before assignment.
6
transport_tls.c:200-213
memory
Socket fd leak on TLS connect failure. When client_connect_tls() fails after TCP connection but before SSL handshake, client_delete() only frees the struct and SSL_CTX — it does NOT close client->file_descriptor. Fix: Call client_disconnect() before client_delete() in error paths at client_send.c:488-494.
7
client_cli.c:156,162
logic
Missing endptr validation for strtoull.--delta-block and --delta-max use strtoull(argv[++i], NULL, 10) which silently ignores parse errors — "abc" produces val=0, failing the range check with a misleading warning. Fix: Pass a non-NULL endptr and check *endptr != '\0'.
8
compression.c:83-84
security
Weak decompression bomb mitigation. When ZSTD_CONTENTSIZE_UNKNOWN, fallback is only 3x compressed size. The realloc-doubling loop could allocate huge amounts before the stream is fully consumed. Fix: Add a hard ceiling checked inside the realloc loop.
STYLE (4)
#
File:Line
Category
Issue
9
log.c:25,35
logic
log_level_strings[log_level] has no bounds check — OOB read if log_level >= 4.
10
compression.c:4-5
build
#include "stdlib.h" uses quotes instead of angle brackets for system headers.
11
file.c:19
build
Duplicate #include "log.h" (lines 13 and 19).
12
protocol.c:137-138
security
bw_tokens/bw_last_refill are not thread-local but modified without synchronization. Safe today (fork-based server, single sender thread), but latent race condition.
VERDICT
[FAIL] — 3 critical issues must be fixed before merge:
server.c: use-after-free when writer thread creation fails (receiver thread not joined)
client_send.c:529: NULL dereference in manifest from str_dup() failure
client_send.c:388: Same NULL dereference in multithreaded path
Note: The __thread io_ssl change (#4) is not a strict blocker since the current code explicitly calls io_set_ssl() in each thread, but AGENTS.md should be updated to reflect this design change. The PR also adds solid improvements: receive timeouts, per-connection memory limits, OOM guards in delta/chunk deserialization, and path traversal detection.
## PR #200 Review — refactor/cli-parser-helpers
**Files reviewed:** 50 | **Dimensions:** code, build, CI, docs, quality
---
### CRITICAL (3)
| # | File:Line | Category | Issue |
|---|-----------|----------|-------|
| 1 | `server.c:138-143` | thread | **Use-after-free on partial thread creation failure.** If `thrd_create(&receiver,...)` succeeds but `thrd_create(&writer,...)` fails, the receiver thread is NOT joined before `pipeline_context_receiver_destroy(context)` is called. The receiver thread continues accessing freed mutexes, queues, and config. **Fix:** Join the receiver thread before destroying the context on writer creation failure. |
| 2 | `client_send.c:529` | memory | **NULL dereference in manifest creation.** `str_dup(p)` can return NULL on allocation failure. `array_list_add(manifest, NULL)` adds NULL to the list. Later, `send_delete_manifest()` calls `send_str(fd, (char*)manifest->items[i])` which calls `strlen(NULL)` — segfault. **Fix:** Check `str_dup()` return value before `array_list_add`; skip or abort on NULL. |
| 3 | `client_send.c:388` | memory | **Same NULL dereference in multithreaded path.** `array_list_add(context->manifest, str_dup(p))` can add NULL to the manifest list, causing the same crash downstream. **Fix:** Same as above — check `str_dup()` return value. |
### WARNING (5)
| # | File:Line | Category | Issue |
|---|-----------|----------|-------|
| 4 | `protocol.c:18` | thread | **`__thread io_ssl` contradicts documented guidance.** AGENTS.md explicitly warns: "io_ssl must NOT be thread-local". While the current code explicitly calls `io_set_ssl()` in each thread (making `__thread` safe), this contradicts documented guidance and makes the code fragile. **Fix:** Update AGENTS.md to document the new per-thread `io_set_ssl()` contract. |
| 5 | `client_cli.c:263-269` | memory | **FILE* leak on multiple `--log-file` arguments.** If `--log-file` is specified twice, the previous `FILE*` in `config->log_file` is overwritten without `fclose()`. **Fix:** Add `if (config->log_file) fclose(config->log_file);` before assignment. |
| 6 | `transport_tls.c:200-213` | memory | **Socket fd leak on TLS connect failure.** When `client_connect_tls()` fails after TCP connection but before SSL handshake, `client_delete()` only frees the struct and SSL_CTX — it does NOT close `client->file_descriptor`. **Fix:** Call `client_disconnect()` before `client_delete()` in error paths at `client_send.c:488-494`. |
| 7 | `client_cli.c:156,162` | logic | **Missing endptr validation for `strtoull`.** `--delta-block` and `--delta-max` use `strtoull(argv[++i], NULL, 10)` which silently ignores parse errors — "abc" produces val=0, failing the range check with a misleading warning. **Fix:** Pass a non-NULL endptr and check `*endptr != '\0'`. |
| 8 | `compression.c:83-84` | security | **Weak decompression bomb mitigation.** When `ZSTD_CONTENTSIZE_UNKNOWN`, fallback is only 3x compressed size. The realloc-doubling loop could allocate huge amounts before the stream is fully consumed. **Fix:** Add a hard ceiling checked inside the realloc loop. |
### STYLE (4)
| # | File:Line | Category | Issue |
|---|-----------|----------|-------|
| 9 | `log.c:25,35` | logic | `log_level_strings[log_level]` has no bounds check — OOB read if `log_level >= 4`. |
| 10 | `compression.c:4-5` | build | `#include "stdlib.h"` uses quotes instead of angle brackets for system headers. |
| 11 | `file.c:19` | build | Duplicate `#include "log.h"` (lines 13 and 19). |
| 12 | `protocol.c:137-138` | security | `bw_tokens`/`bw_last_refill` are not thread-local but modified without synchronization. Safe today (fork-based server, single sender thread), but latent race condition. |
---
### VERDICT
**[FAIL]** — 3 critical issues must be fixed before merge:
1. **server.c:** use-after-free when writer thread creation fails (receiver thread not joined)
2. **client_send.c:529:** NULL dereference in manifest from `str_dup()` failure
3. **client_send.c:388:** Same NULL dereference in multithreaded path
---
**Note:** The `__thread io_ssl` change (#4) is not a strict blocker since the current code explicitly calls `io_set_ssl()` in each thread, but AGENTS.md should be updated to reflect this design change. The PR also adds solid improvements: receive timeouts, per-connection memory limits, OOM guards in delta/chunk deserialization, and path traversal detection.
[C1] src/shared/config.h:131 -- protocol
PROTOCOL_VERSION changed from 2.0.0 to 1.3.0. This is a protocol regression -- any existing peer running 2.0.0 will reject connections from this version, and vice versa. If this is intentional (e.g., resetting version for a new major refactor), the version should be incremented forward (e.g., 3.0.0 or 2.1.0), not reverted. If this is a mistake, restore the previous version.
Fix: Restore PROTOCOL_VERSION to 2.0.0 or increment to a forward-compatible version.
WARNINGS (7)
[W1] src/shared/protocol.c:18 -- thread / AGENTS.md divergence
io_ssl changed from static SSL* io_ssl = NULL; to static __thread SSL* io_ssl;. The project AGENTS.md explicitly states: __thread on shared SSL context: io_ssl must NOT be thread-local -- worker threads inherit the SSL context from the main thread. The current code happens to work because receive_thread and write_thread both explicitly call io_set_ssl(context->ssl), but this contradicts project conventions and will break if any future code path assumes cross-thread SSL visibility.
Fix: Remove __thread from io_ssl to match AGENTS.md guidance, or update AGENTS.md if the new pattern is intentional.
[W2] src/shared/protocol.c:179 -- security
The NULL check in send_str was removed. If any caller passes NULL, strlen(NULL) will segfault. While current callers handle NULL before calling (e.g., config_send uses ternary), removing the defensive check reduces robustness.
Fix: Restore the NULL guard: if (!data) return false;
[W3] src/shared/config.c:133 -- error handling
The NULL check if (config == NULL) return; was removed from config_delete. While current callers check for NULL before calling, this breaks the defensive convention and could crash if a future caller forgets.
Fix: Restore the NULL guard in config_delete.
[W4] src/shared/log.c:22 -- thread safety
log_message uses localtime() which returns a pointer to static storage and is not thread-safe. In multithreaded mode, concurrent calls to log_message from scanner/loader/sender threads can corrupt the timestamp buffer.
Fix: Use localtime_r() instead: struct tm t_buf; localtime_r(&now, &t_buf);
[W5] src/client/client_cli.c:170-177 -- validation
When -c is used with an optional level argument, the value from strtol is cast to int and stored without range validation. Zstd compression levels valid range is 1-22; values outside this range may produce unexpected behavior.
Fix: Add range check before assignment.
[W6] src/shared/protocol.c:20-22 -- thread safety (latent)
bw_tokens and bw_last_refill are static (not __thread), meaning they are shared between threads. bw_throttle modifies them without mutex protection. In the current architecture only the sender thread does IO, so this is not a practical issue, but it is a latent race condition.
Fix: Either make these __thread (each thread has independent bandwidth) or protect them with a mutex.
[W7] src/shared/file.c:86-101 -- error handling
file_load_data allocates file->data->data on line 90 but does not free it or NULL the pointer if file_content_to_buffer returns an unexpected size (line 97). The file is left in an inconsistent state with allocated but invalid data. Callers do destroy the file on failure, so this is not a leak, but it violates the principle of leaving objects in a clean state on error.
Fix: On read failure, add free(file->data->data); file->data->data = NULL; file->data->size = 0; before returning false.
STYLE (2)
[S1] src/shared/utils.c:33-40 -- defensive coding
The bounds check if (pos + part_len + 1 >= buf_size) was removed from mkdir_r. While the total length of tokenized path components can never exceed the original path length (making the check redundant), removing it reduces defensiveness.
[S2] src/shared/utils.c:67-108 -- comments removed
Comments explaining globstar (), single-*, and // pattern behavior were removed. While the code is readable, these comments aid maintenance for a complex recursive function.
SECURITY IMPROVEMENTS (noted positively)
The PR includes several good security hardening measures:
has_path_traversal() function added and used in file_save_to_disk and receive_incremental_check
MAX_DATA_SIZE reduced from 1 GB to 100 MB per message
Path escape detection via realpath comparison in file_save_to_disk
Symlinks skipped in delete_extras_walk to prevent symlink-following attacks
VERDICT
[FAIL] 1 critical issue (protocol version regression) must be resolved before merge.
The protocol version change from 2.0.0 to 1.3.0 will break all existing peers. This must either be restored to 2.0.0 or incremented to a forward-compatible version.
The __thread io_ssl change [W1] contradicts project conventions and should also be addressed, though the current code structure happens to work correctly with it.
## PR #200 Review -- Refactor CLI Parser Helpers
**Branch:** `refactor/cli-parser-helpers`
**Files reviewed:** 43 (.c/.h)
**Dimensions checked:** code, build, CI, docs, quality
---
### CRITICAL (2)
**[C1] `src/shared/config.h:131` -- protocol**
PROTOCOL_VERSION changed from `2.0.0` to `1.3.0`. This is a **protocol regression** -- any existing peer running 2.0.0 will reject connections from this version, and vice versa. If this is intentional (e.g., resetting version for a new major refactor), the version should be incremented forward (e.g., `3.0.0` or `2.1.0`), not reverted. If this is a mistake, restore the previous version.
**Fix:** Restore PROTOCOL_VERSION to `2.0.0` or increment to a forward-compatible version.
---
### WARNINGS (7)
**[W1] `src/shared/protocol.c:18` -- thread / AGENTS.md divergence**
io_ssl changed from `static SSL* io_ssl = NULL;` to `static __thread SSL* io_ssl;`. The project AGENTS.md explicitly states: *__thread on shared SSL context: io_ssl must NOT be thread-local -- worker threads inherit the SSL context from the main thread.* The current code happens to work because receive_thread and write_thread both explicitly call io_set_ssl(context->ssl), but this contradicts project conventions and will break if any future code path assumes cross-thread SSL visibility.
**Fix:** Remove `__thread` from io_ssl to match AGENTS.md guidance, or update AGENTS.md if the new pattern is intentional.
**[W2] `src/shared/protocol.c:179` -- security**
The NULL check in send_str was removed. If any caller passes NULL, strlen(NULL) will segfault. While current callers handle NULL before calling (e.g., config_send uses ternary), removing the defensive check reduces robustness.
**Fix:** Restore the NULL guard: `if (!data) return false;`
**[W3] `src/shared/config.c:133` -- error handling**
The NULL check `if (config == NULL) return;` was removed from config_delete. While current callers check for NULL before calling, this breaks the defensive convention and could crash if a future caller forgets.
**Fix:** Restore the NULL guard in config_delete.
**[W4] `src/shared/log.c:22` -- thread safety**
log_message uses localtime() which returns a pointer to static storage and is not thread-safe. In multithreaded mode, concurrent calls to log_message from scanner/loader/sender threads can corrupt the timestamp buffer.
**Fix:** Use localtime_r() instead: `struct tm t_buf; localtime_r(&now, &t_buf);`
**[W5] `src/client/client_cli.c:170-177` -- validation**
When -c is used with an optional level argument, the value from strtol is cast to int and stored without range validation. Zstd compression levels valid range is 1-22; values outside this range may produce unexpected behavior.
**Fix:** Add range check before assignment.
**[W6] `src/shared/protocol.c:20-22` -- thread safety (latent)**
bw_tokens and bw_last_refill are static (not __thread), meaning they are shared between threads. bw_throttle modifies them without mutex protection. In the current architecture only the sender thread does IO, so this is not a practical issue, but it is a latent race condition.
**Fix:** Either make these __thread (each thread has independent bandwidth) or protect them with a mutex.
**[W7] `src/shared/file.c:86-101` -- error handling**
file_load_data allocates file->data->data on line 90 but does not free it or NULL the pointer if file_content_to_buffer returns an unexpected size (line 97). The file is left in an inconsistent state with allocated but invalid data. Callers do destroy the file on failure, so this is not a leak, but it violates the principle of leaving objects in a clean state on error.
**Fix:** On read failure, add `free(file->data->data); file->data->data = NULL; file->data->size = 0;` before returning false.
---
### STYLE (2)
**[S1] `src/shared/utils.c:33-40` -- defensive coding**
The bounds check `if (pos + part_len + 1 >= buf_size)` was removed from mkdir_r. While the total length of tokenized path components can never exceed the original path length (making the check redundant), removing it reduces defensiveness.
**[S2] `src/shared/utils.c:67-108` -- comments removed**
Comments explaining globstar (**), single-*, and /**/ pattern behavior were removed. While the code is readable, these comments aid maintenance for a complex recursive function.
---
### SECURITY IMPROVEMENTS (noted positively)
The PR includes several good security hardening measures:
- has_path_traversal() function added and used in file_save_to_disk and receive_incremental_check
- MAX_DATA_SIZE reduced from 1 GB to 100 MB per message
- MAX_CONNECTION_MEMORY (1 GB per-connection cap) prevents OOM
- RECEIVE_TIMEOUT_SEC (60s) prevents hung connections
- MAX_FILE_DATA_SIZE (64 MB) and MAX_CHUNK_SIZE (64 MB) prevent OOM from malicious payloads
- MAX_DELTA_BLOCKS / MAX_DELTA_INSTRUCTIONS bounds prevent delta OOM
- Path escape detection via realpath comparison in file_save_to_disk
- Symlinks skipped in delete_extras_walk to prevent symlink-following attacks
---
### VERDICT
**[FAIL] 1 critical issue (protocol version regression) must be resolved before merge.**
The protocol version change from `2.0.0` to `1.3.0` will break all existing peers. This must either be restored to `2.0.0` or incremented to a forward-compatible version.
The __thread io_ssl change [W1] contradicts project conventions and should also be addressed, though the current code structure happens to work correctly with it.
This PR refactors client_cli.c by extracting CLI parser helper functions (parse_positive_int, parse_nonneg_int, set_string_option, set_positive_int_option, set_nonneg_int_option), refactors config_create() to use a no-arg constructor with field-by-field initialization, expands the protocol wire format with many new rsync-parity fields, adds unit tests, and adds new features (parallel scanner, TLS, delta, keepalive, abort, batch check, etc.).
CRITICAL Issues
[CRITICAL-1] src/shared/config.h:131 — Protocol Version Regression PROTOCOL_VERSION was changed from "2.0.0" (on main) to "1.3.0". This is a version downgrade. The wire format in config_send/config_receive was significantly expanded (removed exclude/include patterns from wire, added backup, copy_links, safe_links, copy_unsafe_links, preserve_*, update, inplace, append, delete_excluded, delete_after, max_delete, relative, prune_empty_dirs, temp_dir, partial_dir, suffix, delete_before, checksum, compress_choice). A new client speaking "1.3.0" will connect to a server expecting "2.0.0" and fail the version check, or worse, if versions are mismatched the deserialization will read the wrong fields in the wrong order. Fix: Restore PROTOCOL_VERSION to "2.0.0" or bump to "2.1.0" if the wire format changed. The version MUST match the actual wire format.
[CRITICAL-2] src/shared/config.c:133 — config_delete NULL Guard Removed
The PR removed the if (config == NULL) return; guard from config_delete(). Every caller in the codebase must now guarantee non-NULL, but this violates defensive programming. If any code path ever calls config_delete(NULL) (e.g., error path in config_receive at line 281, or handler() in server.c at line 126), it will crash. Fix: Restore the NULL guard: if (config == NULL) return;
[CRITICAL-3] src/shared/config.c:170-264 — config_send Wire Format Changed Without Version Bump
The config_send function no longer sends exclude_patterns, include_patterns, max_size, or min_size over the wire, but adds 20+ new fields. If a client built from this branch connects to a server built from main (or vice versa), deserialization will read garbage -- the field ordering is completely different. Since the protocol version string is "1.3.0" (downgraded), the version check might pass incorrectly. Fix: Ensure the protocol version is bumped and matches the actual wire format. Consider adding a wire format version field separate from the application version.
WARNING Issues
[WARNING-1] src/client/scanner.c:28-34 — dir_entry_create Path Not NULL-Checked dir_entry_create calls str_dup(path) but does not check if it returns NULL. If allocation fails, the DirEntry has a NULL path field and is returned. When dequeued in open_next_directory (line 100-101), opendir(NULL) is called, causing a crash. Fix: Return NULL from dir_entry_create if str_dup fails, and check the return value of queue_enqueue in directory_scanner_create.
[WARNING-2] src/client/scanner.c:65 — directory_scanner_create Enqueues Without NULL Check queue_enqueue(scanner->directories, dir_entry_create(root_directory, 0)) -- if dir_entry_create returns NULL (OOM), a NULL pointer is enqueued. Later dequeued and dereferenced. Fix: Check dir_entry_create return value before enqueuing.
[WARNING-3] src/shared/multiprocessing.c:29-38 — Resource Leak on Synchronization Init Failure pipeline_context_sender_create initializes mutexes and condition variables sequentially. If mtx_init(&mutex_progress) or cnd_init(&condition_not_empty_loader) fails, the previously initialized mutexes and conditions are not destroyed. Same issue in parallel_scanner_create (scanner.c:326-332). Fix: Track which resources were initialized and destroy them on failure, or use a cleanup pattern.
[WARNING-4] src/client/client_send.c:347-354 — Chunk Memory Leak on Send Failure
In send_chunks_multithreaded, when send_chunk fails (line 347), the function jumps to send_fail without calling chunk_destroy(current_chunk). The chunk was dequeued from the queue (so the queue no longer owns it) but is never freed. Fix: Add chunk_destroy(current_chunk) before the goto send_fail on the error path.
[WARNING-5] src/client/client_cli.c:263-268 — FILE Handle Leak on Duplicate --log-file
If --log-file is specified twice, the first FILE* opened at line 263 is overwritten at line 268 without being closed. config->log_file = lf replaces the old pointer. Fix: Close the previous config->log_file before opening a new one: if (config->log_file) fclose(config->log_file);
[WARNING-6] src/client/client_cli.c:155-160 — strtoull With NULL Endptr --delta-block uses strtoull(argv[++i], NULL, 10) with NULL endptr, so invalid input like "abc" silently returns 0, which then falls through to the warning path. While not exploitable, it is inconsistent with other options that properly check endptr. Fix: Use a non-NULL endptr and validate: char* end; strtoull(argv[++i], &end, 10); if (*end != '\0') ...
[WARNING-7] src/client/client_cli.c:170-178 — Compression Level Not Range-Checked
The -c [level] option calls strtol and stores the result as compression_level without checking that it is in the valid range (1-22 for zstd). Invalid values will cause ZSTD to return errors at compression time. Fix: Add range validation: if (level < 1 || level > 22) { fprintf(stderr, "Error: compression level must be 1-22\n"); return -1; }
[WARNING-8] src/client/client_send.c:629-634 — Thread Creation Failure Does Not Join Running Threads
In send_files_multithreaded, if thrd_create(&loader, ...) fails after thrd_create(&scanner, ...) succeeds, the scanner thread is running and accessing context which is about to be destroyed by pipeline_context_sender_destroy. This is a use-after-free. Fix: If any thread creation fails, join any already-created threads before destroying the context.
[WARNING-9] src/shared/log.c:22 — localtime() Not Thread-Safe log_message uses localtime() which returns a pointer to a static buffer. In a multithreaded server, timestamps could be corrupted when multiple threads log concurrently. Fix: Use localtime_r() instead: struct tm tm_buf; localtime_r(&now, &tm_buf); and use &tm_buf in the format calls.
[WARNING-10] src/client/client_send.c:388 — str_dup NULL Not Checked in Manifest array_list_add(context->manifest, str_dup(p)) -- if str_dup returns NULL, a NULL pointer is added to the manifest. Later, send_delete_manifest iterates and calls send_str(fd, (char*)manifest->items[i]) which would pass NULL to strlen inside send_str, crashing. Fix: Check str_dup return value: char* d = str_dup(p); if (d) array_list_add(context->manifest, d);
STYLE Issues
[STYLE-1] src/client/client_cli.c:585 — Fixed 4096 Buffer for Pattern Files char line[4096] in read_patterns_from_file silently truncates lines longer than 4095 bytes. While reasonable for patterns, it should be documented. Fix: Add a comment noting the 4096-byte line limit, or use dynamic allocation.
[STYLE-2] src/shared/config.c:336-337 — Redundant Field Initialization
Lines like config->show_progress = false; config->dry_run = false; are set in config_receive but were already set to 0 by memset(config, 0, sizeof(*config)) at line 271. The entire block from lines 336-391 could be removed for clarity. Fix: Remove redundant assignments after memset, or add a comment explaining they are kept for documentation.
Verdict
[FAIL] -- 3 critical issues must be fixed before merge:
Protocol version regression (PROTOCOL_VERSION downgraded from 2.0.0 to 1.3.0)
config_delete NULL guard removed (crash risk)
Wire format changed without proper version coordination
The refactoring quality is good -- the helper extraction is clean, tests are added, and the code is more maintainable. But the protocol version issue is a blocking compatibility problem that will break client-server interop.
## PR #200 Review: refactor/cli-parser-helpers
### Summary
This PR refactors `client_cli.c` by extracting CLI parser helper functions (`parse_positive_int`, `parse_nonneg_int`, `set_string_option`, `set_positive_int_option`, `set_nonneg_int_option`), refactors `config_create()` to use a no-arg constructor with field-by-field initialization, expands the protocol wire format with many new rsync-parity fields, adds unit tests, and adds new features (parallel scanner, TLS, delta, keepalive, abort, batch check, etc.).
### CRITICAL Issues
**[CRITICAL-1] src/shared/config.h:131 — Protocol Version Regression**
`PROTOCOL_VERSION` was changed from `"2.0.0"` (on main) to `"1.3.0"`. This is a version *downgrade*. The wire format in `config_send`/`config_receive` was significantly expanded (removed exclude/include patterns from wire, added backup, copy_links, safe_links, copy_unsafe_links, preserve_*, update, inplace, append, delete_excluded, delete_after, max_delete, relative, prune_empty_dirs, temp_dir, partial_dir, suffix, delete_before, checksum, compress_choice). A new client speaking "1.3.0" will connect to a server expecting "2.0.0" and fail the version check, or worse, if versions are mismatched the deserialization will read the wrong fields in the wrong order.
**Fix:** Restore `PROTOCOL_VERSION` to `"2.0.0"` or bump to `"2.1.0"` if the wire format changed. The version MUST match the actual wire format.
**[CRITICAL-2] src/shared/config.c:133 — config_delete NULL Guard Removed**
The PR removed the `if (config == NULL) return;` guard from `config_delete()`. Every caller in the codebase must now guarantee non-NULL, but this violates defensive programming. If any code path ever calls `config_delete(NULL)` (e.g., error path in `config_receive` at line 281, or `handler()` in server.c at line 126), it will crash.
**Fix:** Restore the NULL guard: `if (config == NULL) return;`
**[CRITICAL-3] src/shared/config.c:170-264 — config_send Wire Format Changed Without Version Bump**
The `config_send` function no longer sends exclude_patterns, include_patterns, max_size, or min_size over the wire, but adds 20+ new fields. If a client built from this branch connects to a server built from main (or vice versa), deserialization will read garbage -- the field ordering is completely different. Since the protocol version string is "1.3.0" (downgraded), the version check might pass incorrectly.
**Fix:** Ensure the protocol version is bumped and matches the actual wire format. Consider adding a wire format version field separate from the application version.
### WARNING Issues
**[WARNING-1] src/client/scanner.c:28-34 — dir_entry_create Path Not NULL-Checked**
`dir_entry_create` calls `str_dup(path)` but does not check if it returns NULL. If allocation fails, the DirEntry has a NULL `path` field and is returned. When dequeued in `open_next_directory` (line 100-101), `opendir(NULL)` is called, causing a crash.
**Fix:** Return NULL from `dir_entry_create` if `str_dup` fails, and check the return value of `queue_enqueue` in `directory_scanner_create`.
**[WARNING-2] src/client/scanner.c:65 — directory_scanner_create Enqueues Without NULL Check**
`queue_enqueue(scanner->directories, dir_entry_create(root_directory, 0))` -- if `dir_entry_create` returns NULL (OOM), a NULL pointer is enqueued. Later dequeued and dereferenced.
**Fix:** Check `dir_entry_create` return value before enqueuing.
**[WARNING-3] src/shared/multiprocessing.c:29-38 — Resource Leak on Synchronization Init Failure**
`pipeline_context_sender_create` initializes mutexes and condition variables sequentially. If `mtx_init(&mutex_progress)` or `cnd_init(&condition_not_empty_loader)` fails, the previously initialized mutexes and conditions are not destroyed. Same issue in `parallel_scanner_create` (scanner.c:326-332).
**Fix:** Track which resources were initialized and destroy them on failure, or use a cleanup pattern.
**[WARNING-4] src/client/client_send.c:347-354 — Chunk Memory Leak on Send Failure**
In `send_chunks_multithreaded`, when `send_chunk` fails (line 347), the function jumps to `send_fail` without calling `chunk_destroy(current_chunk)`. The chunk was dequeued from the queue (so the queue no longer owns it) but is never freed.
**Fix:** Add `chunk_destroy(current_chunk)` before the `goto send_fail` on the error path.
**[WARNING-5] src/client/client_cli.c:263-268 — FILE Handle Leak on Duplicate --log-file**
If `--log-file` is specified twice, the first `FILE*` opened at line 263 is overwritten at line 268 without being closed. `config->log_file = lf` replaces the old pointer.
**Fix:** Close the previous `config->log_file` before opening a new one: `if (config->log_file) fclose(config->log_file);`
**[WARNING-6] src/client/client_cli.c:155-160 — strtoull With NULL Endptr**
`--delta-block` uses `strtoull(argv[++i], NULL, 10)` with NULL endptr, so invalid input like "abc" silently returns 0, which then falls through to the warning path. While not exploitable, it is inconsistent with other options that properly check endptr.
**Fix:** Use a non-NULL endptr and validate: `char* end; strtoull(argv[++i], &end, 10); if (*end != '\0') ...`
**[WARNING-7] src/client/client_cli.c:170-178 — Compression Level Not Range-Checked**
The `-c [level]` option calls `strtol` and stores the result as `compression_level` without checking that it is in the valid range (1-22 for zstd). Invalid values will cause ZSTD to return errors at compression time.
**Fix:** Add range validation: `if (level < 1 || level > 22) { fprintf(stderr, "Error: compression level must be 1-22\n"); return -1; }`
**[WARNING-8] src/client/client_send.c:629-634 — Thread Creation Failure Does Not Join Running Threads**
In `send_files_multithreaded`, if `thrd_create(&loader, ...)` fails after `thrd_create(&scanner, ...)` succeeds, the scanner thread is running and accessing `context` which is about to be destroyed by `pipeline_context_sender_destroy`. This is a use-after-free.
**Fix:** If any thread creation fails, join any already-created threads before destroying the context.
**[WARNING-9] src/shared/log.c:22 — localtime() Not Thread-Safe**
`log_message` uses `localtime()` which returns a pointer to a static buffer. In a multithreaded server, timestamps could be corrupted when multiple threads log concurrently.
**Fix:** Use `localtime_r()` instead: `struct tm tm_buf; localtime_r(&now, &tm_buf);` and use `&tm_buf` in the format calls.
**[WARNING-10] src/client/client_send.c:388 — str_dup NULL Not Checked in Manifest**
`array_list_add(context->manifest, str_dup(p))` -- if `str_dup` returns NULL, a NULL pointer is added to the manifest. Later, `send_delete_manifest` iterates and calls `send_str(fd, (char*)manifest->items[i])` which would pass NULL to `strlen` inside `send_str`, crashing.
**Fix:** Check `str_dup` return value: `char* d = str_dup(p); if (d) array_list_add(context->manifest, d);`
### STYLE Issues
**[STYLE-1] src/client/client_cli.c:585 — Fixed 4096 Buffer for Pattern Files**
`char line[4096]` in `read_patterns_from_file` silently truncates lines longer than 4095 bytes. While reasonable for patterns, it should be documented.
**Fix:** Add a comment noting the 4096-byte line limit, or use dynamic allocation.
**[STYLE-2] src/shared/config.c:336-337 — Redundant Field Initialization**
Lines like `config->show_progress = false; config->dry_run = false;` are set in `config_receive` but were already set to 0 by `memset(config, 0, sizeof(*config))` at line 271. The entire block from lines 336-391 could be removed for clarity.
**Fix:** Remove redundant assignments after memset, or add a comment explaining they are kept for documentation.
### Verdict
**[FAIL]** -- 3 critical issues must be fixed before merge:
1. Protocol version regression (`PROTOCOL_VERSION` downgraded from 2.0.0 to 1.3.0)
2. `config_delete` NULL guard removed (crash risk)
3. Wire format changed without proper version coordination
The refactoring quality is good -- the helper extraction is clean, tests are added, and the code is more maintainable. But the protocol version issue is a blocking compatibility problem that will break client-server interop.
[1] src/shared/file.c:108 — protocol / compression mismatch compression_should_skip() causes sender to skip compression for certain file extensions (.jpg, .png, .zip, .gz, etc.), sending raw data. However, the receiver unconditionally decompresses when config->use_compression == true. This causes zstd decompression failures for every pre-compressed file type when compression is enabled.
Fix: Add compression_should_skip(file->path) check on the receiver side before calling data_decompress, OR remove the sender-side skip and let zstd handle already-compressed data, OR send a per-file flag indicating whether data is compressed.
[2] src/shared/log.c:22 — thread safety regression
Replaced thread-safe localtime_r() with non-thread-safe localtime(). The server forks child processes that each call log_message(), and the multithreaded receiver path also calls log_message() from receive_thread and write_thread. localtime() returns a pointer to a shared static buffer, causing data races when multiple threads call it concurrently.
[3] src/shared/protocol.c:18 — AGENTS.md pitfall
Changed io_ssl from static SSL* to static __thread SSL*. AGENTS.md explicitly warns against this: "io_ssl must NOT be thread-local — worker threads inherit the SSL context from the main thread." The new architecture explicitly passes SSL* through PipelineContextReceiver and calls io_set_ssl() per-thread, so __thread works in practice. However, the project documentation explicitly warns against this.
Fix: Update AGENTS.md to reflect the new explicit-passing architecture, OR remove __thread and keep the old shared model.
[4] src/shared/protocol.c:179-180 — defensive NULL check removed
Removed the NULL check from send_str(). Now strlen(data) is called unconditionally. While current callers protect with ternary operators (config->field ? config->field : ""), removing the defensive check makes the function fragile for future callers. Any NULL input causes undefined behavior.
Fix: Restore the NULL check, or add an assertion: assert(data != NULL && "send_str: NULL data");
[5] src/shared/protocol.c:24 — memory accounting leak total_allocated_bytes is incremented in receive_data() but never decremented. Each connection thread has its own counter (__thread), but over a long-lived connection receiving many files, the counter grows without bound. Once it exceeds MAX_CONNECTION_MEMORY (1 GB), all subsequent receive_data() calls fail, even if previous data has been freed.
Fix: Decrement total_allocated_bytes in data_destroy() or add a corresponding io_account_free(size) call.
[6] src/shared/compression.c:27-28 — validation removed
Removed compression level clamping that previously ensured levels stay in [1, 22]. Out-of-range levels (e.g., -c 99 via the -c flag which uses strtol without range validation) are now passed directly to ZSTD_compress, which returns an error for levels > ZSTD_maxCLevel(). This changes behavior from "auto-clamp" to "fail".
Fix: Restore clamping in data_compress(), or add validation in CLI parsing for both -c and --compress-level.
[7] src/shared/config.h:81 — protocol version downgrade PROTOCOL_VERSION changed from "2.0.0" to "1.3.0". This is a version downgrade that will cause mutual rejection between new clients and old servers (and vice versa). The wire protocol has also changed significantly (new fields, removed exclude/include patterns). This looks like a merge conflict resolution error.
Fix: Bump to "3.0.0" (or a new major version) to reflect the breaking protocol changes, not downgrade.
[8] src/shared/protocol.c:12,14 — capacity regression MAX_DATA_SIZE reduced from 1 GB to 100 MB. Combined with the removal of file_send_streaming() (the old streaming path for files > 64 MB), files between 100 MB and 1 GB that were previously transferable may now fail. The multithreaded path uses sendfile for large files (bypassing send_data), but the single-threaded path and compressed transfers go through send_data which enforces the 100 MB limit.
Fix: Verify that all large-file paths (single-threaded, compressed, chunked) can handle files > 100 MB, or restore a higher limit.
[9] src/shared/file.c:97-99 — cleanup removed file_load_data() no longer cleans up the partially-read buffer on failure. While the buffer is eventually freed by file_destroy(), leaving a partially-filled buffer with stale size is less defensive. Callers who fail to check the return value could access corrupt data.
Fix: Restore the cleanup code for robustness.
[10] src/shared/transport_tls.c:97-114 — potential infinite loop
The SSL handshake retry loop retries indefinitely on SSL_ERROR_WANT_READ / SSL_ERROR_WANT_WRITE. With blocking sockets and a misbehaving peer (or network), this could spin forever.
Fix: Add a retry counter or deadline-based timeout to the handshake loop.
[11] src/shared/transport_tcp.c:244-256 — safety net removed client_delete() no longer calls client_disconnect(). While all current callers properly call client_disconnect() first, this removes a safety net. If a future caller forgets, the socket file descriptor leaks.
Fix: Either restore client_disconnect() inside client_delete(), or add a check and close if fd >= 0.
[12] src/shared/utils.c:10-46 — bounds check removed mkdir_r() removed the buffer overflow guard. While the buffer size (strlen(path) + 2) is mathematically sufficient for reconstructing the path from its strtok_r components, removing the explicit bounds check reduces defense-in-depth.
Fix: Restore the bounds check or add an assertion.
[13] src/shared/file.c:280 — sparse file offset overflow to_disk() passes data_size - 1 (unsigned long long) to fseek() which takes long. If data_size - 1 > LONG_MAX, the offset silently overflows, causing the sparse file write to seek to the wrong position.
Fix: Add a check: if (data_size - 1 > LONG_MAX) { ok = false; goto done; }
[14] src/shared/log.c:33-40 — concurrent file writes log_fp is a global FILE* written to by multiple threads/processes without any locking. Concurrent fprintf(log_fp, ...) calls can interleave output, producing garbled log lines.
Fix: Add a mutex around the log file write block, or use flock() on the file descriptor.
STYLE
[15] src/shared/file.c:646 — sendfile count truncation file_send_sendfile() passes file_size - offset (unsigned long long) directly to sendfile() which expects size_t. On 32-bit systems, this truncates.
[16] src/shared/file.c:97-100 — incomplete read not logged with details
When bytes_read != file->data->size, the error message doesn't log how many bytes were actually read vs expected. This makes debugging I/O failures harder.
Fix:log_message(LOG_LEVEL_ERROR, "Read %zu of %zu bytes from %s", bytes_read, file->data->size, file->path);
VERDICT
[FAIL] 2 critical issues must be fixed before merge:
compression_should_skip protocol mismatch — sender/receiver disagree on compression state
localtime() thread-safety regression in log_message()
## PR Review
```
=== PR REVIEW SUMMARY ===
Branch: refactor/cli-parser-helpers
Files reviewed: 45
Issues found: 16
CRITICAL: 2
WARNING: 12
STYLE: 2
```
### CRITICAL
**[1] `src/shared/file.c:108` — protocol / compression mismatch**
`compression_should_skip()` causes sender to skip compression for certain file extensions (.jpg, .png, .zip, .gz, etc.), sending raw data. However, the receiver unconditionally decompresses when `config->use_compression == true`. This causes zstd decompression failures for every pre-compressed file type when compression is enabled.
> **Fix:** Add `compression_should_skip(file->path)` check on the receiver side before calling `data_decompress`, OR remove the sender-side skip and let zstd handle already-compressed data, OR send a per-file flag indicating whether data is compressed.
**[2] `src/shared/log.c:22` — thread safety regression**
Replaced thread-safe `localtime_r()` with non-thread-safe `localtime()`. The server forks child processes that each call `log_message()`, and the multithreaded receiver path also calls `log_message()` from `receive_thread` and `write_thread`. `localtime()` returns a pointer to a shared static buffer, causing data races when multiple threads call it concurrently.
> **Fix:** Restore `localtime_r()`:
```c
struct tm result_buf;
const struct tm* t = localtime_r(&now, &result_buf);
```
### WARNING
**[3] `src/shared/protocol.c:18` — AGENTS.md pitfall**
Changed `io_ssl` from `static SSL*` to `static __thread SSL*`. AGENTS.md explicitly warns against this: "io_ssl must NOT be thread-local — worker threads inherit the SSL context from the main thread." The new architecture explicitly passes SSL* through PipelineContextReceiver and calls `io_set_ssl()` per-thread, so `__thread` works in practice. However, the project documentation explicitly warns against this.
> **Fix:** Update AGENTS.md to reflect the new explicit-passing architecture, OR remove `__thread` and keep the old shared model.
**[4] `src/shared/protocol.c:179-180` — defensive NULL check removed**
Removed the NULL check from `send_str()`. Now `strlen(data)` is called unconditionally. While current callers protect with ternary operators (`config->field ? config->field : ""`), removing the defensive check makes the function fragile for future callers. Any NULL input causes undefined behavior.
> **Fix:** Restore the NULL check, or add an assertion: `assert(data != NULL && "send_str: NULL data");`
**[5] `src/shared/protocol.c:24` — memory accounting leak**
`total_allocated_bytes` is incremented in `receive_data()` but never decremented. Each connection thread has its own counter (`__thread`), but over a long-lived connection receiving many files, the counter grows without bound. Once it exceeds `MAX_CONNECTION_MEMORY` (1 GB), all subsequent `receive_data()` calls fail, even if previous data has been freed.
> **Fix:** Decrement `total_allocated_bytes` in `data_destroy()` or add a corresponding `io_account_free(size)` call.
**[6] `src/shared/compression.c:27-28` — validation removed**
Removed compression level clamping that previously ensured levels stay in [1, 22]. Out-of-range levels (e.g., `-c 99` via the `-c` flag which uses strtol without range validation) are now passed directly to ZSTD_compress, which returns an error for levels > ZSTD_maxCLevel(). This changes behavior from "auto-clamp" to "fail".
> **Fix:** Restore clamping in `data_compress()`, or add validation in CLI parsing for both `-c` and `--compress-level`.
**[7] `src/shared/config.h:81` — protocol version downgrade**
`PROTOCOL_VERSION` changed from `"2.0.0"` to `"1.3.0"`. This is a version downgrade that will cause mutual rejection between new clients and old servers (and vice versa). The wire protocol has also changed significantly (new fields, removed exclude/include patterns). This looks like a merge conflict resolution error.
> **Fix:** Bump to `"3.0.0"` (or a new major version) to reflect the breaking protocol changes, not downgrade.
**[8] `src/shared/protocol.c:12,14` — capacity regression**
`MAX_DATA_SIZE` reduced from 1 GB to 100 MB. Combined with the removal of `file_send_streaming()` (the old streaming path for files > 64 MB), files between 100 MB and 1 GB that were previously transferable may now fail. The multithreaded path uses sendfile for large files (bypassing `send_data`), but the single-threaded path and compressed transfers go through `send_data` which enforces the 100 MB limit.
> **Fix:** Verify that all large-file paths (single-threaded, compressed, chunked) can handle files > 100 MB, or restore a higher limit.
**[9] `src/shared/file.c:97-99` — cleanup removed**
`file_load_data()` no longer cleans up the partially-read buffer on failure. While the buffer is eventually freed by `file_destroy()`, leaving a partially-filled buffer with stale size is less defensive. Callers who fail to check the return value could access corrupt data.
> **Fix:** Restore the cleanup code for robustness.
**[10] `src/shared/transport_tls.c:97-114` — potential infinite loop**
The SSL handshake retry loop retries indefinitely on `SSL_ERROR_WANT_READ` / `SSL_ERROR_WANT_WRITE`. With blocking sockets and a misbehaving peer (or network), this could spin forever.
> **Fix:** Add a retry counter or deadline-based timeout to the handshake loop.
**[11] `src/shared/transport_tcp.c:244-256` — safety net removed**
`client_delete()` no longer calls `client_disconnect()`. While all current callers properly call `client_disconnect()` first, this removes a safety net. If a future caller forgets, the socket file descriptor leaks.
> **Fix:** Either restore `client_disconnect()` inside `client_delete()`, or add a check and close if fd >= 0.
**[12] `src/shared/utils.c:10-46` — bounds check removed**
`mkdir_r()` removed the buffer overflow guard. While the buffer size (`strlen(path) + 2`) is mathematically sufficient for reconstructing the path from its `strtok_r` components, removing the explicit bounds check reduces defense-in-depth.
> **Fix:** Restore the bounds check or add an assertion.
**[13] `src/shared/file.c:280` — sparse file offset overflow**
`to_disk()` passes `data_size - 1` (unsigned long long) to `fseek()` which takes `long`. If `data_size - 1 > LONG_MAX`, the offset silently overflows, causing the sparse file write to seek to the wrong position.
> **Fix:** Add a check: `if (data_size - 1 > LONG_MAX) { ok = false; goto done; }`
**[14] `src/shared/log.c:33-40` — concurrent file writes**
`log_fp` is a global `FILE*` written to by multiple threads/processes without any locking. Concurrent `fprintf(log_fp, ...)` calls can interleave output, producing garbled log lines.
> **Fix:** Add a mutex around the log file write block, or use `flock()` on the file descriptor.
### STYLE
**[15] `src/shared/file.c:646` — sendfile count truncation**
`file_send_sendfile()` passes `file_size - offset` (unsigned long long) directly to `sendfile()` which expects `size_t`. On 32-bit systems, this truncates.
> **Fix:** Add bounds check or cast: `size_t send_count = (size_t)MIN(file_size - offset, SIZE_MAX);`
**[16] `src/shared/file.c:97-100` — incomplete read not logged with details**
When `bytes_read != file->data->size`, the error message doesn't log how many bytes were actually read vs expected. This makes debugging I/O failures harder.
> **Fix:** `log_message(LOG_LEVEL_ERROR, "Read %zu of %zu bytes from %s", bytes_read, file->data->size, file->path);`
---
### VERDICT
**[FAIL]** 2 critical issues must be fixed before merge:
1. `compression_should_skip` protocol mismatch — sender/receiver disagree on compression state
2. `localtime()` thread-safety regression in `log_message()`
- Fix FILE* leak when --log-file specified multiple times
- Add strtoull endptr validation for --delta-block/--delta-max
- Add range validation (1-22) for -c compression level
progress_thread uninitialized if thrd_create fails
STILL PRESENT
6
Signal handler calls async-signal-unsafe server_delete()
STILL PRESENT
7
Protocol version downgraded 2.0.0 → 1.3.0
STILL PRESENT
8
Compression level validation only in CLI, not config_receive()
PARTIALLY FIXED
9
--delta-block/--delta-max strtoull validation
FIXED
10
Log file not closed on re-assignment
FIXED
11
receive_chunk_enqueue return type change
FIXED
New Issues
#
File
Severity
Issue
1
config.c:26
WARNING
config_receive() accepts wire compression_level without range validation
2
config.c:133
WARNING
config_delete() NULL check guard removed
Still Present — Details
[1] CRITICAL — compression_should_skip() protocol mismatch
Sender conditionally compresses based on file extension via compression_should_skip(), but file_receive() (line 684-692), receive_incremental_check() (line 505-513), and receive_delta_file() (line 304-314) ALL unconditionally decompress when config->use_compression is true. Transferring a .jpg with compression enabled will send raw data but the receiver will attempt zstd decompression → data corruption/crash.
Fix: Either (a) send a "compressed" flag per-file on the wire so the receiver knows whether to decompress, or (b) always compress every file and remove compression_should_skip().
[2] CRITICAL — localtime() thread-safety regression log_message() uses localtime() which returns a pointer to a shared static buffer. In multithreaded mode, multiple threads calling log_message() concurrently corrupt the shared static tm struct.
[3] WARNING — __thread SSL* io_ssl breaks TLS for sender threads
Changed from static SSL* io_ssl = NULL to static __thread SSL* io_ssl. Per AGENTS.md: "io_ssl must NOT be thread-local — worker threads inherit the SSL context from the main thread." The sender thread in send_chunks_multithreaded() never calls io_set_ssl(), so io_ssl is NULL for sender threads, causing send_n_data() to use raw write() instead of SSL_write() → TLS bypassed entirely.
Fix: Revert to static SSL* io_ssl = NULL; (non-thread-local).
[4] WARNING — progress_thread used uninitialized if thrd_create fails
If thrd_create(&progress, ...) fails, progress is uninitialized. Later thrd_join(progress, NULL) is called unconditionally → undefined behavior.
Fix: Initialize progress = 0 and add a flag tracking whether it was created. Only join if created successfully.
[5] WARNING — Server signal handler calls async-signal-unsafe functions
Signal handler calls server_delete() which calls free(), close(), mtx_destroy() etc. — all NOT async-signal-safe. Calling them from a signal handler is undefined behavior.
Fix: Set a flag and let the main loop handle cleanup, or use only async-signal-safe functions (write, _exit).
[6] WARNING — Protocol version downgraded from "2.0.0" to "1.3.0"
This will cause mutual rejection between new clients and old servers. Looks like a merge conflict resolution error.
Fix: Bump to a new version (e.g., "2.1.0") to reflect the breaking protocol change.
VERDICT
[FAIL] — 2 critical issues must be fixed before merge:
compression_should_skip() protocol mismatch — will corrupt data when transferring already-compressed files with compression enabled
localtime() thread-safety regression — will cause data races in multithreaded logging
## Re-Review
```
=== PR REVIEW SUMMARY ===
Branch: refactor/cli-parser-helpers
Files reviewed: 46
Issues found: 18 (2 critical, 14 warning, 2 style)
```
### Previous Issues — Status
| # | Issue | Status |
|---|-------|--------|
| 1 | `compression_should_skip()` protocol mismatch | **STILL PRESENT** |
| 2 | `localtime()` thread-safety regression | **STILL PRESENT** |
| 3 | `send_str()` NULL check removed | **STILL PRESENT** |
| 4 | `__thread io_ssl` breaks sender TLS | **STILL PRESENT** |
| 5 | `progress_thread` uninitialized if `thrd_create` fails | **STILL PRESENT** |
| 6 | Signal handler calls async-signal-unsafe `server_delete()` | **STILL PRESENT** |
| 7 | Protocol version downgraded 2.0.0 → 1.3.0 | **STILL PRESENT** |
| 8 | Compression level validation only in CLI, not `config_receive()` | **PARTIALLY FIXED** |
| 9 | `--delta-block`/`--delta-max` strtoull validation | **FIXED** |
| 10 | Log file not closed on re-assignment | **FIXED** |
| 11 | `receive_chunk_enqueue` return type change | **FIXED** |
### New Issues
| # | File | Severity | Issue |
|---|------|----------|-------|
| 1 | `config.c:26` | WARNING | `config_receive()` accepts wire compression_level without range validation |
| 2 | `config.c:133` | WARNING | `config_delete()` NULL check guard removed |
### Still Present — Details
**[1] CRITICAL — `compression_should_skip()` protocol mismatch**
Sender conditionally compresses based on file extension via `compression_should_skip()`, but `file_receive()` (line 684-692), `receive_incremental_check()` (line 505-513), and `receive_delta_file()` (line 304-314) ALL unconditionally decompress when `config->use_compression` is true. Transferring a .jpg with compression enabled will send raw data but the receiver will attempt zstd decompression → data corruption/crash.
> Fix: Either (a) send a "compressed" flag per-file on the wire so the receiver knows whether to decompress, or (b) always compress every file and remove `compression_should_skip()`.
**[2] CRITICAL — `localtime()` thread-safety regression**
`log_message()` uses `localtime()` which returns a pointer to a shared static buffer. In multithreaded mode, multiple threads calling `log_message()` concurrently corrupt the shared static tm struct.
> Fix: Restore `localtime_r()`:
```c
struct tm result_buf;
const struct tm* t = localtime_r(&now, &result_buf);
```
**[3] WARNING — `__thread` SSL\* io_ssl breaks TLS for sender threads**
Changed from `static SSL* io_ssl = NULL` to `static __thread SSL* io_ssl`. Per AGENTS.md: "io_ssl must NOT be thread-local — worker threads inherit the SSL context from the main thread." The sender thread in `send_chunks_multithreaded()` never calls `io_set_ssl()`, so `io_ssl` is NULL for sender threads, causing `send_n_data()` to use raw `write()` instead of `SSL_write()` → TLS bypassed entirely.
> Fix: Revert to `static SSL* io_ssl = NULL;` (non-thread-local).
**[4] WARNING — `progress_thread` used uninitialized if `thrd_create` fails**
If `thrd_create(&progress, ...)` fails, `progress` is uninitialized. Later `thrd_join(progress, NULL)` is called unconditionally → undefined behavior.
> Fix: Initialize `progress = 0` and add a flag tracking whether it was created. Only join if created successfully.
**[5] WARNING — Server signal handler calls async-signal-unsafe functions**
Signal handler calls `server_delete()` which calls `free()`, `close()`, `mtx_destroy()` etc. — all NOT async-signal-safe. Calling them from a signal handler is undefined behavior.
> Fix: Set a flag and let the main loop handle cleanup, or use only async-signal-safe functions (write, _exit).
**[6] WARNING — Protocol version downgraded from "2.0.0" to "1.3.0"**
This will cause mutual rejection between new clients and old servers. Looks like a merge conflict resolution error.
> Fix: Bump to a new version (e.g., "2.1.0") to reflect the breaking protocol change.
### VERDICT
**[FAIL]** — 2 critical issues must be fixed before merge:
1. `compression_should_skip()` protocol mismatch — will corrupt data when transferring already-compressed files with compression enabled
2. `localtime()` thread-safety regression — will cause data races in multithreaded logging
- Restore PROTOCOL_VERSION to a forward-compatible 2.1.0 and document wire format
- Add NULL guard to config_delete
- Add pipeline cancellation flag and cancellation-aware queue helper
- Join running threads before destroying pipeline contexts on creation failure
- Fix NULL dereference and memory leaks in manifest/chunk handling
- Fix TLS/TCP socket fd leak on connect error paths
- Add compression-level range validation (1-22)
- Close previous log file before opening a new one
- Use getline for unbounded pattern-file lines
- Fix thread-unsafe localtime() and add log level bounds check
- Fix file_load_data to clean up data on read size mismatch
- Add hard ceiling to decompression buffer growth
- Fix mkdir_r bounds check and restore glob comments
- Add send_str NULL guard and mutex-protect bandwidth limiter
- Update AGENTS.md for per-thread io_ssl contract
- Fix memory leak in delta_deserialize() on BLOCK_MATCH error path
- Fix memory leak on malloc failure for literal data
- Add port range validation (1-65535) for -p/--port and --server-port
- Restore -V/--version flag with usage text
- Use MAX_DATA_PAYLOAD_SIZE consistently (remove local MAX_DATA_SIZE)
- Fix integer truncation in config_send/config_receive for delta_block_size
- Add log messages for malloc failures
- Extract config_set_defaults() helper to eliminate duplication
- Add 10 new CLI tests for argument parsing
TapTap
merged commit d529360c01 into dev2026-08-08 20:02:10 +02:00
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.
Changes
set_string_option(),set_positive_int_option(),set_nonneg_int_option()helpers to eliminate duplicated alloc/free/assign patternsset_string_option()callsset_positive/nonneg_int_option()--info/--debug: replaceatoi()with validated parse--max-size/--min-size: addstrtoull()error checking--chunk-size: validate input with error message on failureStats
81 insertions, 110 deletions (net -29 lines). All 25 unit tests + 38 integration tests pass. Clean build with
-Wextra -Wpedantic -Werror.PR #200 Review — refactor/cli-parser-helpers
Files reviewed: 50 | Dimensions: code, build, CI, docs, quality
CRITICAL (3)
server.c:138-143thrd_create(&receiver,...)succeeds butthrd_create(&writer,...)fails, the receiver thread is NOT joined beforepipeline_context_receiver_destroy(context)is called. The receiver thread continues accessing freed mutexes, queues, and config. Fix: Join the receiver thread before destroying the context on writer creation failure.client_send.c:529str_dup(p)can return NULL on allocation failure.array_list_add(manifest, NULL)adds NULL to the list. Later,send_delete_manifest()callssend_str(fd, (char*)manifest->items[i])which callsstrlen(NULL)— segfault. Fix: Checkstr_dup()return value beforearray_list_add; skip or abort on NULL.client_send.c:388array_list_add(context->manifest, str_dup(p))can add NULL to the manifest list, causing the same crash downstream. Fix: Same as above — checkstr_dup()return value.WARNING (5)
protocol.c:18__thread io_sslcontradicts documented guidance. AGENTS.md explicitly warns: "io_ssl must NOT be thread-local". While the current code explicitly callsio_set_ssl()in each thread (making__threadsafe), this contradicts documented guidance and makes the code fragile. Fix: Update AGENTS.md to document the new per-threadio_set_ssl()contract.client_cli.c:263-269--log-filearguments.* If--log-fileis specified twice, the previousFILE*inconfig->log_fileis overwritten withoutfclose(). Fix: Addif (config->log_file) fclose(config->log_file);before assignment.transport_tls.c:200-213client_connect_tls()fails after TCP connection but before SSL handshake,client_delete()only frees the struct and SSL_CTX — it does NOT closeclient->file_descriptor. Fix: Callclient_disconnect()beforeclient_delete()in error paths atclient_send.c:488-494.client_cli.c:156,162strtoull.--delta-blockand--delta-maxusestrtoull(argv[++i], NULL, 10)which silently ignores parse errors — "abc" produces val=0, failing the range check with a misleading warning. Fix: Pass a non-NULL endptr and check*endptr != '\0'.compression.c:83-84ZSTD_CONTENTSIZE_UNKNOWN, fallback is only 3x compressed size. The realloc-doubling loop could allocate huge amounts before the stream is fully consumed. Fix: Add a hard ceiling checked inside the realloc loop.STYLE (4)
log.c:25,35log_level_strings[log_level]has no bounds check — OOB read iflog_level >= 4.compression.c:4-5#include "stdlib.h"uses quotes instead of angle brackets for system headers.file.c:19#include "log.h"(lines 13 and 19).protocol.c:137-138bw_tokens/bw_last_refillare not thread-local but modified without synchronization. Safe today (fork-based server, single sender thread), but latent race condition.VERDICT
[FAIL] — 3 critical issues must be fixed before merge:
str_dup()failureNote: The
__thread io_sslchange (#4) is not a strict blocker since the current code explicitly callsio_set_ssl()in each thread, but AGENTS.md should be updated to reflect this design change. The PR also adds solid improvements: receive timeouts, per-connection memory limits, OOM guards in delta/chunk deserialization, and path traversal detection.PR #200 Review -- Refactor CLI Parser Helpers
Branch:
refactor/cli-parser-helpersFiles reviewed: 43 (.c/.h)
Dimensions checked: code, build, CI, docs, quality
CRITICAL (2)
[C1]
src/shared/config.h:131-- protocolPROTOCOL_VERSION changed from
2.0.0to1.3.0. This is a protocol regression -- any existing peer running 2.0.0 will reject connections from this version, and vice versa. If this is intentional (e.g., resetting version for a new major refactor), the version should be incremented forward (e.g.,3.0.0or2.1.0), not reverted. If this is a mistake, restore the previous version.Fix: Restore PROTOCOL_VERSION to
2.0.0or increment to a forward-compatible version.WARNINGS (7)
[W1]
src/shared/protocol.c:18-- thread / AGENTS.md divergenceio_ssl changed from
static SSL* io_ssl = NULL;tostatic __thread SSL* io_ssl;. The project AGENTS.md explicitly states: __thread on shared SSL context: io_ssl must NOT be thread-local -- worker threads inherit the SSL context from the main thread. The current code happens to work because receive_thread and write_thread both explicitly call io_set_ssl(context->ssl), but this contradicts project conventions and will break if any future code path assumes cross-thread SSL visibility.Fix: Remove
__threadfrom io_ssl to match AGENTS.md guidance, or update AGENTS.md if the new pattern is intentional.[W2]
src/shared/protocol.c:179-- securityThe NULL check in send_str was removed. If any caller passes NULL, strlen(NULL) will segfault. While current callers handle NULL before calling (e.g., config_send uses ternary), removing the defensive check reduces robustness.
Fix: Restore the NULL guard:
if (!data) return false;[W3]
src/shared/config.c:133-- error handlingThe NULL check
if (config == NULL) return;was removed from config_delete. While current callers check for NULL before calling, this breaks the defensive convention and could crash if a future caller forgets.Fix: Restore the NULL guard in config_delete.
[W4]
src/shared/log.c:22-- thread safetylog_message uses localtime() which returns a pointer to static storage and is not thread-safe. In multithreaded mode, concurrent calls to log_message from scanner/loader/sender threads can corrupt the timestamp buffer.
Fix: Use localtime_r() instead:
struct tm t_buf; localtime_r(&now, &t_buf);[W5]
src/client/client_cli.c:170-177-- validationWhen -c is used with an optional level argument, the value from strtol is cast to int and stored without range validation. Zstd compression levels valid range is 1-22; values outside this range may produce unexpected behavior.
Fix: Add range check before assignment.
[W6]
src/shared/protocol.c:20-22-- thread safety (latent)bw_tokens and bw_last_refill are static (not __thread), meaning they are shared between threads. bw_throttle modifies them without mutex protection. In the current architecture only the sender thread does IO, so this is not a practical issue, but it is a latent race condition.
Fix: Either make these __thread (each thread has independent bandwidth) or protect them with a mutex.
[W7]
src/shared/file.c:86-101-- error handlingfile_load_data allocates file->data->data on line 90 but does not free it or NULL the pointer if file_content_to_buffer returns an unexpected size (line 97). The file is left in an inconsistent state with allocated but invalid data. Callers do destroy the file on failure, so this is not a leak, but it violates the principle of leaving objects in a clean state on error.
Fix: On read failure, add
free(file->data->data); file->data->data = NULL; file->data->size = 0;before returning false.STYLE (2)
[S1]
src/shared/utils.c:33-40-- defensive codingThe bounds check
if (pos + part_len + 1 >= buf_size)was removed from mkdir_r. While the total length of tokenized path components can never exceed the original path length (making the check redundant), removing it reduces defensiveness.[S2]
src/shared/utils.c:67-108-- comments removedComments explaining globstar (), single-*, and // pattern behavior were removed. While the code is readable, these comments aid maintenance for a complex recursive function.
SECURITY IMPROVEMENTS (noted positively)
The PR includes several good security hardening measures:
VERDICT
[FAIL] 1 critical issue (protocol version regression) must be resolved before merge.
The protocol version change from
2.0.0to1.3.0will break all existing peers. This must either be restored to2.0.0or incremented to a forward-compatible version.The __thread io_ssl change [W1] contradicts project conventions and should also be addressed, though the current code structure happens to work correctly with it.
PR #200 Review: refactor/cli-parser-helpers
Summary
This PR refactors
client_cli.cby extracting CLI parser helper functions (parse_positive_int,parse_nonneg_int,set_string_option,set_positive_int_option,set_nonneg_int_option), refactorsconfig_create()to use a no-arg constructor with field-by-field initialization, expands the protocol wire format with many new rsync-parity fields, adds unit tests, and adds new features (parallel scanner, TLS, delta, keepalive, abort, batch check, etc.).CRITICAL Issues
[CRITICAL-1] src/shared/config.h:131 — Protocol Version Regression
PROTOCOL_VERSIONwas changed from"2.0.0"(on main) to"1.3.0". This is a version downgrade. The wire format inconfig_send/config_receivewas significantly expanded (removed exclude/include patterns from wire, added backup, copy_links, safe_links, copy_unsafe_links, preserve_*, update, inplace, append, delete_excluded, delete_after, max_delete, relative, prune_empty_dirs, temp_dir, partial_dir, suffix, delete_before, checksum, compress_choice). A new client speaking "1.3.0" will connect to a server expecting "2.0.0" and fail the version check, or worse, if versions are mismatched the deserialization will read the wrong fields in the wrong order.Fix: Restore
PROTOCOL_VERSIONto"2.0.0"or bump to"2.1.0"if the wire format changed. The version MUST match the actual wire format.[CRITICAL-2] src/shared/config.c:133 — config_delete NULL Guard Removed
The PR removed the
if (config == NULL) return;guard fromconfig_delete(). Every caller in the codebase must now guarantee non-NULL, but this violates defensive programming. If any code path ever callsconfig_delete(NULL)(e.g., error path inconfig_receiveat line 281, orhandler()in server.c at line 126), it will crash.Fix: Restore the NULL guard:
if (config == NULL) return;[CRITICAL-3] src/shared/config.c:170-264 — config_send Wire Format Changed Without Version Bump
The
config_sendfunction no longer sends exclude_patterns, include_patterns, max_size, or min_size over the wire, but adds 20+ new fields. If a client built from this branch connects to a server built from main (or vice versa), deserialization will read garbage -- the field ordering is completely different. Since the protocol version string is "1.3.0" (downgraded), the version check might pass incorrectly.Fix: Ensure the protocol version is bumped and matches the actual wire format. Consider adding a wire format version field separate from the application version.
WARNING Issues
[WARNING-1] src/client/scanner.c:28-34 — dir_entry_create Path Not NULL-Checked
dir_entry_createcallsstr_dup(path)but does not check if it returns NULL. If allocation fails, the DirEntry has a NULLpathfield and is returned. When dequeued inopen_next_directory(line 100-101),opendir(NULL)is called, causing a crash.Fix: Return NULL from
dir_entry_createifstr_dupfails, and check the return value ofqueue_enqueueindirectory_scanner_create.[WARNING-2] src/client/scanner.c:65 — directory_scanner_create Enqueues Without NULL Check
queue_enqueue(scanner->directories, dir_entry_create(root_directory, 0))-- ifdir_entry_createreturns NULL (OOM), a NULL pointer is enqueued. Later dequeued and dereferenced.Fix: Check
dir_entry_createreturn value before enqueuing.[WARNING-3] src/shared/multiprocessing.c:29-38 — Resource Leak on Synchronization Init Failure
pipeline_context_sender_createinitializes mutexes and condition variables sequentially. Ifmtx_init(&mutex_progress)orcnd_init(&condition_not_empty_loader)fails, the previously initialized mutexes and conditions are not destroyed. Same issue inparallel_scanner_create(scanner.c:326-332).Fix: Track which resources were initialized and destroy them on failure, or use a cleanup pattern.
[WARNING-4] src/client/client_send.c:347-354 — Chunk Memory Leak on Send Failure
In
send_chunks_multithreaded, whensend_chunkfails (line 347), the function jumps tosend_failwithout callingchunk_destroy(current_chunk). The chunk was dequeued from the queue (so the queue no longer owns it) but is never freed.Fix: Add
chunk_destroy(current_chunk)before thegoto send_failon the error path.[WARNING-5] src/client/client_cli.c:263-268 — FILE Handle Leak on Duplicate --log-file
If
--log-fileis specified twice, the firstFILE*opened at line 263 is overwritten at line 268 without being closed.config->log_file = lfreplaces the old pointer.Fix: Close the previous
config->log_filebefore opening a new one:if (config->log_file) fclose(config->log_file);[WARNING-6] src/client/client_cli.c:155-160 — strtoull With NULL Endptr
--delta-blockusesstrtoull(argv[++i], NULL, 10)with NULL endptr, so invalid input like "abc" silently returns 0, which then falls through to the warning path. While not exploitable, it is inconsistent with other options that properly check endptr.Fix: Use a non-NULL endptr and validate:
char* end; strtoull(argv[++i], &end, 10); if (*end != '\0') ...[WARNING-7] src/client/client_cli.c:170-178 — Compression Level Not Range-Checked
The
-c [level]option callsstrtoland stores the result ascompression_levelwithout checking that it is in the valid range (1-22 for zstd). Invalid values will cause ZSTD to return errors at compression time.Fix: Add range validation:
if (level < 1 || level > 22) { fprintf(stderr, "Error: compression level must be 1-22\n"); return -1; }[WARNING-8] src/client/client_send.c:629-634 — Thread Creation Failure Does Not Join Running Threads
In
send_files_multithreaded, ifthrd_create(&loader, ...)fails afterthrd_create(&scanner, ...)succeeds, the scanner thread is running and accessingcontextwhich is about to be destroyed bypipeline_context_sender_destroy. This is a use-after-free.Fix: If any thread creation fails, join any already-created threads before destroying the context.
[WARNING-9] src/shared/log.c:22 — localtime() Not Thread-Safe
log_messageuseslocaltime()which returns a pointer to a static buffer. In a multithreaded server, timestamps could be corrupted when multiple threads log concurrently.Fix: Use
localtime_r()instead:struct tm tm_buf; localtime_r(&now, &tm_buf);and use&tm_bufin the format calls.[WARNING-10] src/client/client_send.c:388 — str_dup NULL Not Checked in Manifest
array_list_add(context->manifest, str_dup(p))-- ifstr_dupreturns NULL, a NULL pointer is added to the manifest. Later,send_delete_manifestiterates and callssend_str(fd, (char*)manifest->items[i])which would pass NULL tostrleninsidesend_str, crashing.Fix: Check
str_dupreturn value:char* d = str_dup(p); if (d) array_list_add(context->manifest, d);STYLE Issues
[STYLE-1] src/client/client_cli.c:585 — Fixed 4096 Buffer for Pattern Files
char line[4096]inread_patterns_from_filesilently truncates lines longer than 4095 bytes. While reasonable for patterns, it should be documented.Fix: Add a comment noting the 4096-byte line limit, or use dynamic allocation.
[STYLE-2] src/shared/config.c:336-337 — Redundant Field Initialization
Lines like
config->show_progress = false; config->dry_run = false;are set inconfig_receivebut were already set to 0 bymemset(config, 0, sizeof(*config))at line 271. The entire block from lines 336-391 could be removed for clarity.Fix: Remove redundant assignments after memset, or add a comment explaining they are kept for documentation.
Verdict
[FAIL] -- 3 critical issues must be fixed before merge:
PROTOCOL_VERSIONdowngraded from 2.0.0 to 1.3.0)config_deleteNULL guard removed (crash risk)The refactoring quality is good -- the helper extraction is clean, tests are added, and the code is more maintainable. But the protocol version issue is a blocking compatibility problem that will break client-server interop.
PR Review
CRITICAL
[1]
src/shared/file.c:108— protocol / compression mismatchcompression_should_skip()causes sender to skip compression for certain file extensions (.jpg, .png, .zip, .gz, etc.), sending raw data. However, the receiver unconditionally decompresses whenconfig->use_compression == true. This causes zstd decompression failures for every pre-compressed file type when compression is enabled.[2]
src/shared/log.c:22— thread safety regressionReplaced thread-safe
localtime_r()with non-thread-safelocaltime(). The server forks child processes that each calllog_message(), and the multithreaded receiver path also callslog_message()fromreceive_threadandwrite_thread.localtime()returns a pointer to a shared static buffer, causing data races when multiple threads call it concurrently.WARNING
[3]
src/shared/protocol.c:18— AGENTS.md pitfallChanged
io_sslfromstatic SSL*tostatic __thread SSL*. AGENTS.md explicitly warns against this: "io_ssl must NOT be thread-local — worker threads inherit the SSL context from the main thread." The new architecture explicitly passes SSL* through PipelineContextReceiver and callsio_set_ssl()per-thread, so__threadworks in practice. However, the project documentation explicitly warns against this.[4]
src/shared/protocol.c:179-180— defensive NULL check removedRemoved the NULL check from
send_str(). Nowstrlen(data)is called unconditionally. While current callers protect with ternary operators (config->field ? config->field : ""), removing the defensive check makes the function fragile for future callers. Any NULL input causes undefined behavior.[5]
src/shared/protocol.c:24— memory accounting leaktotal_allocated_bytesis incremented inreceive_data()but never decremented. Each connection thread has its own counter (__thread), but over a long-lived connection receiving many files, the counter grows without bound. Once it exceedsMAX_CONNECTION_MEMORY(1 GB), all subsequentreceive_data()calls fail, even if previous data has been freed.[6]
src/shared/compression.c:27-28— validation removedRemoved compression level clamping that previously ensured levels stay in [1, 22]. Out-of-range levels (e.g.,
-c 99via the-cflag which uses strtol without range validation) are now passed directly to ZSTD_compress, which returns an error for levels > ZSTD_maxCLevel(). This changes behavior from "auto-clamp" to "fail".[7]
src/shared/config.h:81— protocol version downgradePROTOCOL_VERSIONchanged from"2.0.0"to"1.3.0". This is a version downgrade that will cause mutual rejection between new clients and old servers (and vice versa). The wire protocol has also changed significantly (new fields, removed exclude/include patterns). This looks like a merge conflict resolution error.[8]
src/shared/protocol.c:12,14— capacity regressionMAX_DATA_SIZEreduced from 1 GB to 100 MB. Combined with the removal offile_send_streaming()(the old streaming path for files > 64 MB), files between 100 MB and 1 GB that were previously transferable may now fail. The multithreaded path uses sendfile for large files (bypassingsend_data), but the single-threaded path and compressed transfers go throughsend_datawhich enforces the 100 MB limit.[9]
src/shared/file.c:97-99— cleanup removedfile_load_data()no longer cleans up the partially-read buffer on failure. While the buffer is eventually freed byfile_destroy(), leaving a partially-filled buffer with stale size is less defensive. Callers who fail to check the return value could access corrupt data.[10]
src/shared/transport_tls.c:97-114— potential infinite loopThe SSL handshake retry loop retries indefinitely on
SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITE. With blocking sockets and a misbehaving peer (or network), this could spin forever.[11]
src/shared/transport_tcp.c:244-256— safety net removedclient_delete()no longer callsclient_disconnect(). While all current callers properly callclient_disconnect()first, this removes a safety net. If a future caller forgets, the socket file descriptor leaks.[12]
src/shared/utils.c:10-46— bounds check removedmkdir_r()removed the buffer overflow guard. While the buffer size (strlen(path) + 2) is mathematically sufficient for reconstructing the path from itsstrtok_rcomponents, removing the explicit bounds check reduces defense-in-depth.[13]
src/shared/file.c:280— sparse file offset overflowto_disk()passesdata_size - 1(unsigned long long) tofseek()which takeslong. Ifdata_size - 1 > LONG_MAX, the offset silently overflows, causing the sparse file write to seek to the wrong position.[14]
src/shared/log.c:33-40— concurrent file writeslog_fpis a globalFILE*written to by multiple threads/processes without any locking. Concurrentfprintf(log_fp, ...)calls can interleave output, producing garbled log lines.STYLE
[15]
src/shared/file.c:646— sendfile count truncationfile_send_sendfile()passesfile_size - offset(unsigned long long) directly tosendfile()which expectssize_t. On 32-bit systems, this truncates.[16]
src/shared/file.c:97-100— incomplete read not logged with detailsWhen
bytes_read != file->data->size, the error message doesn't log how many bytes were actually read vs expected. This makes debugging I/O failures harder.VERDICT
[FAIL] 2 critical issues must be fixed before merge:
compression_should_skipprotocol mismatch — sender/receiver disagree on compression statelocaltime()thread-safety regression inlog_message()Re-Review
Previous Issues — Status
compression_should_skip()protocol mismatchlocaltime()thread-safety regressionsend_str()NULL check removed__thread io_sslbreaks sender TLSprogress_threaduninitialized ifthrd_createfailsserver_delete()config_receive()--delta-block/--delta-maxstrtoull validationreceive_chunk_enqueuereturn type changeNew Issues
config.c:26config_receive()accepts wire compression_level without range validationconfig.c:133config_delete()NULL check guard removedStill Present — Details
[1] CRITICAL —
compression_should_skip()protocol mismatchSender conditionally compresses based on file extension via
compression_should_skip(), butfile_receive()(line 684-692),receive_incremental_check()(line 505-513), andreceive_delta_file()(line 304-314) ALL unconditionally decompress whenconfig->use_compressionis true. Transferring a .jpg with compression enabled will send raw data but the receiver will attempt zstd decompression → data corruption/crash.[2] CRITICAL —
localtime()thread-safety regressionlog_message()useslocaltime()which returns a pointer to a shared static buffer. In multithreaded mode, multiple threads callinglog_message()concurrently corrupt the shared static tm struct.[3] WARNING —
__threadSSL* io_ssl breaks TLS for sender threadsChanged from
static SSL* io_ssl = NULLtostatic __thread SSL* io_ssl. Per AGENTS.md: "io_ssl must NOT be thread-local — worker threads inherit the SSL context from the main thread." The sender thread insend_chunks_multithreaded()never callsio_set_ssl(), soio_sslis NULL for sender threads, causingsend_n_data()to use rawwrite()instead ofSSL_write()→ TLS bypassed entirely.[4] WARNING —
progress_threadused uninitialized ifthrd_createfailsIf
thrd_create(&progress, ...)fails,progressis uninitialized. Laterthrd_join(progress, NULL)is called unconditionally → undefined behavior.[5] WARNING — Server signal handler calls async-signal-unsafe functions
Signal handler calls
server_delete()which callsfree(),close(),mtx_destroy()etc. — all NOT async-signal-safe. Calling them from a signal handler is undefined behavior.[6] WARNING — Protocol version downgraded from "2.0.0" to "1.3.0"
This will cause mutual rejection between new clients and old servers. Looks like a merge conflict resolution error.
VERDICT
[FAIL] — 2 critical issues must be fixed before merge:
compression_should_skip()protocol mismatch — will corrupt data when transferring already-compressed files with compression enabledlocaltime()thread-safety regression — will cause data races in multithreaded loggingbd9a1409f6to78fabcd78178fabcd781to05a74770ca