Merge remaining 4 PRs: memory safety, refactoring, test coverage, integration cleanup #93
Reference in New Issue
Block a user
Delete Branch "merge-all"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Combines all fixes from the remaining 4 open PRs into one branch for streamlined merging:
All conflicts resolved. CI passes (22 tests, lint, clang-format, cppcheck, STRICT_WARNINGS).
=== PR REVIEW SUMMARY ===
Branch: merge-all
Files reviewed: 42 (27 source files + 15 test/header files)
Issues found: 12
CRITICAL: 3
WARNING: 6
STYLE: 3
=== ISSUES ===
[1] src/client/client_cli.c:73 — CRITICAL (memory)
config_create() return value is never checked for NULL. If memory allocation
fails inside config_create, the returned pointer is NULL, and the very next
write to config->use_compression (line 89) will dereference NULL, causing a
segfault.
Fix: Add
if (config == NULL) { exit_code = 1; goto cleanup; }immediatelyafter line 73.
[2] src/shared/config.c:339-344 — CRITICAL (memory)
The error handler at label
error:does NOT freeexclude_patterns,include_patterns, their individual strings,tls_cert,tls_key,tls_ca, orssh_destination. If an error occurs after exclude_patternshas been fully allocated (e.g., include_patterns malloc fails at line 302),
all previously allocated pattern strings and arrays are leaked.
Fix: Extend the error handler to free all dynamically-allocated fields
(exclude_patterns + strings, include_patterns + strings, tls_cert,
tls_key, tls_ca, ssh_destination) using the same pattern as config_delete().
[3] src/shared/file.c:629 — CRITICAL (protocol)
In file_send_sendfile(), the count argument to sendfile() is computed as
(file_size - offset) where both are unsigned long long, but sendfile()
accepts size_t. On 32-bit platforms size_t is 32 bits, so a file larger
than 4 GB will have its count silently truncated, causing sendfile to
transfer only (count mod 2^32) bytes. The offset will not advance to
file_size, and the loop spins forever (or sends incorrect data).
Fix: Cap each sendfile iteration at SIZE_MAX (or a safe limit like 2^31)
and loop accordingly.
[4] src/shared/log.c:18 — WARNING (error handling)
localtime_r() can return NULL if time() fails (returns (time_t)-1). The
return value is not checked. Dereferencing t->tm_year on a NULL pointer
will crash.
Fix: Add
if (t == NULL) return;after line 18.[5] src/shared/protocol.c:13 — WARNING (thread safety)
io_read_fd and io_write_fd are declared __thread, but io_ssl is NOT
thread-local. If multiple threads perform SSL I/O simultaneously (e.g.,
in the multithreaded receiver pipeline), the io_ssl variable will be
shared across threads with no synchronization, causing data races and
potentially corrupted SSL state.
Fix: Either make io_ssl thread-local as well, or add mutex protection.
[6] src/shared/config.c:263-266 — WARNING (security)
The overflow check
(size_t)ec > SIZE_MAX / sizeof(char*)is technicallycorrect, but the error message says "integer overflow" when it's actually
a SIZE_MAX overflow check. More importantly, MAX_PATTERN_COUNT (10000) is
the first guard; the SIZE_MAX check is redundant on 64-bit (10000 * 8 =
80000, far below SIZE_MAX). Consider removing the redundant check or
clarifying the comment.
[7] src/shared/file.c:110-112 — WARNING (error handling)
In file_load_data(), when file_content_to_buffer() reads a different number
of bytes than expected, the function returns false but the previously
allocated
file->data->databuffer is not freed. It is still owned bythe File struct, so this only matters if the caller does not destroy the
file on error. Currently all callers do destroy the file, but this is a
latent bug if new callers are added.
Fix: Reset
file->data->data = NULLafter freeing on error, or free it.[8] src/server/server.c:251-253 — WARNING (memory)
If server_listen() returns for any reason other than g_tcp_cleanup_requested
being set (e.g., listen() fails silently, though accept_loop logs the error),
the g_server struct is leaked because it is only deleted under the
g_server_cleanup_requested path. On normal signal-triggered shutdown this
works correctly, but unexpected early returns from accept_loop will leak.
Fix: Always delete g_server after the loop unconditionally, regardless of
the cleanup flag.
[9] src/shared/file.c:136-145 — WARNING (error handling)
In file_send_streaming(), if fread() returns a short count (nread != to_read)
and ferror() is true, the error is logged but no STATUS_ERROR is sent to
the receiver. The receiver will be left waiting for more data, causing a
hang. Same issue at line 147 if send_n_data fails during streaming — the
receiver will hang.
Fix: Send a STATUS_ERROR to the receiver before closing the connection,
or close the connection to signal error.
[10] src/client/scanner.c:94 — STYLE (code quality)
The result of str_dup(root_directory) is passed directly to
queue_enqueue() without a NULL check. If str_dup fails (OOM), a NULL
pointer is enqueued. While open_next_directory() will handle the NULL by
failing opendir(NULL), the error is silently swallowed with only a
perror().
Fix: Add a NULL check and return NULL from directory_scanner_create_full.
[11] src/shared/metadata.h:23 — STYLE (code quality)
The constant FILE_METADATA_WIRE_SIZE computes to 28 bytes (3×int32_t +
2×int64_t) but this does NOT include the
presentint32_t field that isalways sent first. The actual wire size for present metadata is 32 bytes.
The 28-byte value is correct for the "after present" portion, but the
name is misleading.
Fix: Rename to FILE_METADATA_DATA_WIRE_SIZE or add a comment clarifying
that the present field is excluded.
[12] src/shared/transport_ssh.c:121 — STYLE (code quality)
ssh_user is a fixed 512-byte stack buffer. If user@host exceeds 511 chars,
snprintf silently truncates it. While this is extremely unlikely in
practice, the function should at minimum log a warning on truncation or
dynamically size the buffer.
Fix: Check snprintf return value and log a warning if truncation occurred.
=== VERDICT ===
FAIL — 3 critical issues must be fixed before merging.
The critical issues are:
Additionally, the 6 warnings should be addressed before merging,
particularly the log.c localtime_r NULL deref and the protocol.c
non-thread-local io_ssl variable.
The codebase has strong path traversal prevention, proper metadata
permission stripping (SUID/SGID), and generally good error handling
patterns. These issues are focused in specific areas.
Re-review: ALL 12 ISSUES FIXED ✅
All previously reported issues have been addressed:
Critical (3/3 fixed)
Warning (6/6 fixed)
Style (3/3 fixed)
Additional fix
CI: ✅ All green (lint, build, 22 unit tests, 39 integration tests, ASan, UBSan, Valgrind, fuzz-build, coverage)
Verdict: FULLY APPROVED