refactor: extract CLI parser helpers, fix validation bugs #200

Merged

TapTap merged 5 commits from refactor/cli-parser-helpers into dev 2026-08-08 20:02:10 +02:00

Conversation 5 Commits 5 Files Changed 19

+652 -294

TapTap commented 2026-08-01 18:13:16 +02:00

Owner

Copy Link

Copy Source

Changes

• Add set_string_option(), set_positive_int_option(), set_nonneg_int_option() helpers to eliminate duplicated alloc/free/assign patterns
• Replace 14 string-option branches with set_string_option() calls
• Replace 7 integer-option branches with set_positive/nonneg_int_option()
• Fix --info/--debug: replace atoi() with validated parse
• Fix --max-size/--min-size: add strtoull() error checking
• Fix --chunk-size: validate input with error message on failure

Stats

81 insertions, 110 deletions (net -29 lines). All 25 unit tests + 38 integration tests pass. Clean build with -Wextra -Wpedantic -Werror.

## Changes - Add `set_string_option()`, `set_positive_int_option()`, `set_nonneg_int_option()` helpers to eliminate duplicated alloc/free/assign patterns - Replace 14 string-option branches with `set_string_option()` calls - Replace 7 integer-option branches with `set_positive/nonneg_int_option()` - Fix `--info`/`--debug`: replace `atoi()` with validated parse - Fix `--max-size`/`--min-size`: add `strtoull()` error checking - Fix `--chunk-size`: validate input with error message on failure ## Stats 81 insertions, 110 deletions (net -29 lines). All 25 unit tests + 38 integration tests pass. Clean build with `-Wextra -Wpedantic -Werror`.

TapTap added 1 commit 2026-08-01 18:13:16 +02:00

refactor: extract CLI parser helpers, fix validation bugs ... 5eaea9d92e

- Add set_string_option(), set_positive_int_option(), set_nonneg_int_option()
  helpers to eliminate duplicated alloc/free/assign patterns
- Replace 14 string-option branches with set_string_option() calls
- Replace 7 integer-option branches with set_positive/nonneg_int_option()
- Fix --info/--debug: replace atoi() with validated parse
- Fix --max-size/--min-size: add strtoull() error checking
- Fix --chunk-size: validate input with error message on failure

TapTap added 1 commit 2026-08-01 18:14:57 +02:00

merge: resolve conflict with dev, apply helpers to new options bf91c5b588

TapTap commented 2026-08-01 18:21:02 +02:00

Author

Owner

Copy Link

Copy Source

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.

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

TapTap commented 2026-08-01 18:27:29 +02:00

Author

Owner

Copy Link

Copy Source

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.

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

TapTap commented 2026-08-01 18:31:26 +02:00

Author

Owner

Copy Link

Copy Source

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.

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

TapTap commented 2026-08-01 18:39:08 +02:00

Author

Owner

Copy Link

Copy Source

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

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

## 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()`

TapTap added 1 commit 2026-08-01 18:42:04 +02:00

fix: address PR review issues in client_cli.c ... 1064ae6c19

- 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

TapTap commented 2026-08-02 09:01:59 +02:00

Author

Owner

Copy Link

Copy Source

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

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

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

TapTap referenced this issue from a commit 2026-08-02 09:21:12 +02:00

fix: address PR #200 review issues

TapTap referenced this issue from a commit 2026-08-02 09:25:02 +02:00

fix: address PR #200 review issues

TapTap force-pushed refactor/cli-parser-helpers from bd9a1409f6 to 78fabcd781 2026-08-02 09:25:02 +02:00 Compare

TapTap referenced this issue from a commit 2026-08-02 09:34:55 +02:00

fix: address PR #200 review issues

TapTap added 1 commit 2026-08-02 09:34:55 +02:00

fix: address PR #200 review issues ... 05a74770ca

- 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

TapTap force-pushed refactor/cli-parser-helpers from 78fabcd781 to 05a74770ca 2026-08-02 09:34:55 +02:00 Compare

TapTap referenced this issue from a commit 2026-08-08 20:01:53 +02:00

fix: address PR #200 review issues

TapTap added 1 commit 2026-08-08 20:01:53 +02:00

fix: address PR #200 review issues ... 8743d7f522

- 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 dev 2026-08-08 20:02:10 +02:00

TapTap deleted branch refactor/cli-parser-helpers 2026-08-08 20:02:23 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Defect or bug

enhancement

Feature or improvement

memory

Memory safety issue

needs-triage

Needs human review

performance

Performance issue

quality

Code quality improvement

security

Security vulnerability

threading

Threading or concurrency

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

TapTap

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TapTap/FastSync#200