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