Fix memory/null safety bugs (#74, #72, #69, #64, #60, #50, #49, #65) #91

Closed

TapTap wants to merge 0 commits from fix/memory-safety into main

pull from: fix/memory-safety

merge into: TapTap:main

TapTap:dev

TapTap:evaluation-of-codebase-by-human

TapTap:integration/all-fixes-v2

TapTap:fix/features-v2

TapTap:fix/bugs-v2

TapTap:fix/quality-v2

TapTap:fix/security-v2

TapTap:main

TapTap:integration/all-fixes

TapTap:fix/test-skip-tls

TapTap:fix/rsync-parity-all

TapTap:fix/test-coverage-all

TapTap:fix/refactoring-all

TapTap:fix/bugs-all

TapTap:fix/security-all

TapTap:integration/2026-07-29-batch-fix

TapTap:fix/refactor-cli-config

TapTap:fix/null-checks-cli

TapTap:merge-all-v2

TapTap:revert-pr-143-merge

TapTap:fix/protocol-improvements

TapTap:fix/cli-features

TapTap:fix/tests-and-docs

TapTap:fix/security-hardening

TapTap:fix/performance

TapTap:fix/test-coverage

TapTap:fix/integration-cleanup

TapTap:fix/refactoring

TapTap:ci/tier1-review-automation

TapTap:tls-transport

TapTap:add-gh-dependency

TapTap:add-readme

Conversation 3 Commits - Files Changed -

TapTap commented 2026-07-20 19:43:18 +02:00

Owner

Copy Link

Copy Source

Fixes 8 memory/null safety bugs:\n- #74: data_create_empty malloc(0) UB\n- #72: receive_str() no size limit DoS\n- #69: send_str() NULL segfault\n- #64: Scanner pattern ownership\n- #60: SSH argv overflow\n- #50: mkdir_r() buffer overflow\n- #49: chunk_size wire truncation\n- #65: Compression 32-bit truncation

Fixes 8 memory/null safety bugs:\n- #74: data_create_empty malloc(0) UB\n- #72: receive_str() no size limit DoS\n- #69: send_str() NULL segfault\n- #64: Scanner pattern ownership\n- #60: SSH argv overflow\n- #50: mkdir_r() buffer overflow\n- #49: chunk_size wire truncation\n- #65: Compression 32-bit truncation

TapTap added 1 commit 2026-07-20 19:43:19 +02:00

fix: memory/null safety bugs — issues #74, #72, #69, #64, #60, #50, #49, #65 990c4362af

TapTap added 1 commit 2026-07-20 19:48:32 +02:00

ci: trigger CI on PR 788d3c7bea

TapTap added 1 commit 2026-07-20 19:53:43 +02:00

fix: cppcheck and clang-format fixes ba0afd4152

TapTap added 1 commit 2026-07-20 20:11:25 +02:00

ci: re-trigger after review fixes 5e8d0a2dbf

TapTap added 1 commit 2026-07-20 20:28:18 +02:00

ci: re-trigger after fixes e3bd7a8cdf

TapTap added 1 commit 2026-07-20 20:41:18 +02:00

ci: re-trigger after cppcheck fixes 9042dfcfa9

TapTap added 1 commit 2026-07-20 20:48:24 +02:00

fix: auto-detect valgrind to skip fork tests 262a436264

TapTap added 1 commit 2026-07-20 22:04:48 +02:00

ci: trigger CI 83c61aadc2

TapTap commented 2026-07-20 22:13:31 +02:00

Author

Owner

Copy Link

Copy Source

=== CODE QUALITY REVIEW ===

Severity: MEDIUM

1. Unused variable path2_pointer in path_cat (src/shared/utils.c:168)
   char* path2_pointer = path2; is assigned, incremented at line 169 (path2_pointer += 1;), but never read. Dead code left from refactoring. The pointer arithmetic on path2_pointer has no effect.

2. Inconsistent include style (src/shared/data.c:3)
   Uses #include "stdlib.h" (quotes, local include) instead of #include <stdlib.h> (angle brackets, system include). The PR fixed the same issue in compression.c but not in data.c.

3. Inconsistent parameter constness in path_cat (src/shared/utils.c:161)
   path1 is const char* but path2 is char* (mutable). The function modifies path2 pointer via local pointer arithmetic — should use a local const char* variable instead.

4. Wrong printf format specifier in send_data (src/shared/protocol.c:181)
   %lld used for unsigned long long data_size; should be %llu.

5. Wrong printf format specifier in receive_data (src/shared/protocol.c:196)
   Same %lld for unsigned long long — should be %llu.

6. DRY violation — deep-copy logic duplication (src/client/scanner.c:28-78)
   The deep-copy for exclude_patterns (lines 28-48) and include_patterns (lines 52-78) is nearly identical. Could be refactored into a helper function (deep_copy_str_array) to reduce code duplication and error-prone cleanup paths.

7. Long function / high cyclomatic complexity: directory_scanner_next (src/client/scanner.c:135-224)
   89-line function with 4-5 levels of nesting. Handles directory iteration, readdir, stat, pattern matching, size filtering, and chunk accumulation — all in one function. Hard to follow and maintain.

8. str_dup uses strcpy instead of memcpy (src/shared/utils.c:66)
   Inconsistency with the rest of the PR which replaced strcpy with memcpy in mkdir_r. Using memcpy is more consistent (known string length).

=== SECURITY REVIEW ===

Severity: MEDIUM (no critical vulnerabilities found)

1. Unbounded size in receive_data (src/shared/protocol.c:185-197)
   malloc((size_t)size) where size is an unsigned long long received from the network. On 32-bit systems (where size_t is 32-bit), a large unsigned long long value truncates silently. While the truncated value is used consistently for both malloc and receive_n_data, this could enable memory exhaustion attacks. Consider adding a bounds check: if ((size_t)size != size) return NULL; and/or a MAX_DATA_SIZE limit.

2. Path traversal via symlinks (src/client/scanner.c:160-167)
   entry->d_name is used with stat() (not lstat()), which follows symlinks. A symlink pointing outside the scanned directory (e.g., ../../../etc/passwd) would be followed and included in the chunk data. If the scanner is used with untrusted directories, this is a path traversal risk. Document this behavior or add validation.

3. Valgrind detection reads partial /proc/self/maps (tests/test_utils.h:12-21)
   Only reads the first 4095 bytes. If vgpreload appears later in the maps file (unlikely but possible), the detection produces a false negative. Consider using getenv("LD_PRELOAD") or valgrind.h RUNNING_ON_VALGRIND macro instead.

4. data_create frees caller pointer on failure without documentation (src/shared/data.c:27-37)
   When malloc for the Data struct fails, data_create calls free(data), consuming the caller's buffer. This is a "pass ownership" contract that is not documented. All current callers handle this correctly, but future callers may double-free if unaware.

=== GENERAL REVIEW ===

Severity: LOW (logic is correct; minor issues)

1. Overall logic correctness: PASS

   • Deep-copy of exclude/include patterns correctly frees partially-allocated resources on failure.
   • data_create_empty(0) fix correctly handles malloc(0) UB.
   • receive_str size cap (MAX_STRING_SIZE = 10MB) prevents unbounded allocation. Good security fix.
   • transport_ssh.c dynamic argv array (32 slots vs hardcoded 16) with bounds checks prevents buffer overflow.
   • mkdir_r replaces strcpy with memcpy and bounds checks — eliminates buffer overflow.

2. Thread safety concern: bwlimit state is not thread-safe (src/shared/protocol.c:15-17)
   bw_tokens and bw_last_refill are global variables (not __thread). If multiple threads call send_n_data/receive_n_data concurrently, the bandwidth limiter state will race. Pre-existing issue, not introduced by this PR.

3. delete_extras_walk all_removed logic (src/shared/utils.c:106-155)
   Complex state tracking that is correct but fragile. The rmdir with ENOENT ignoring is correct, but an explanatory comment would improve maintainability.

4. transport_ssh.c: no error message before _exit(1) on allocation failure (line 129-130)
   If calloc for ssh_argv fails, the child process silently exits with _exit(1). No error message is printed, making debugging difficult.

5. Test quality: GOOD

   • New test test_data_create_empty_zero covers the malloc(0) fix.
   • New test test_receive_str_oversized covers the MAX_STRING_SIZE cap.
   • Valgrind detection via /proc/self/maps replaces fragile environment variable check.

=== VERDICT ===
PASS with recommendations

The PR correctly fixes several memory safety issues (malloc(0) UB, buffer overflow in mkdir_r, array bounds in ssh transport, missing NULL checks, unbounded string allocation). Code quality is generally good with clean style, consistent error handling, and proper resource cleanup on failure paths.

The issues found are minor (unused variable, wrong printf format specifiers, DRY duplication, include style inconsistency) and do not warrant rejection. I recommend addressing them in a follow-up cleanup.

Key recommendations to address:

1. Remove unused path2_pointer in utils.c:168
2. Fix %lld --> %llu in protocol.c:181,196
3. Fix #include "stdlib.h" --> #include <stdlib.h> in data.c:3
4. Add size bounds check in receive_data for 32-bit safety
5. Refactor scanner.c deep-copy duplication

=== CODE QUALITY REVIEW === **Severity: MEDIUM** 1. **Unused variable `path2_pointer` in `path_cat` (src/shared/utils.c:168)** `char* path2_pointer = path2;` is assigned, incremented at line 169 (`path2_pointer += 1;`), but never read. Dead code left from refactoring. The pointer arithmetic on `path2_pointer` has no effect. 2. **Inconsistent include style (src/shared/data.c:3)** Uses `#include "stdlib.h"` (quotes, local include) instead of `#include <stdlib.h>` (angle brackets, system include). The PR fixed the same issue in `compression.c` but not in `data.c`. 3. **Inconsistent parameter constness in `path_cat` (src/shared/utils.c:161)** `path1` is `const char*` but `path2` is `char*` (mutable). The function modifies `path2` pointer via local pointer arithmetic — should use a local `const char*` variable instead. 4. **Wrong printf format specifier in `send_data` (src/shared/protocol.c:181)** `%lld` used for `unsigned long long data_size`; should be `%llu`. 5. **Wrong printf format specifier in `receive_data` (src/shared/protocol.c:196)** Same `%lld` for `unsigned long long` — should be `%llu`. 6. **DRY violation — deep-copy logic duplication (src/client/scanner.c:28-78)** The deep-copy for `exclude_patterns` (lines 28-48) and `include_patterns` (lines 52-78) is nearly identical. Could be refactored into a helper function (`deep_copy_str_array`) to reduce code duplication and error-prone cleanup paths. 7. **Long function / high cyclomatic complexity: `directory_scanner_next` (src/client/scanner.c:135-224)** 89-line function with 4-5 levels of nesting. Handles directory iteration, readdir, stat, pattern matching, size filtering, and chunk accumulation — all in one function. Hard to follow and maintain. 8. **`str_dup` uses `strcpy` instead of `memcpy` (src/shared/utils.c:66)** Inconsistency with the rest of the PR which replaced `strcpy` with `memcpy` in `mkdir_r`. Using `memcpy` is more consistent (known string length). === SECURITY REVIEW === **Severity: MEDIUM (no critical vulnerabilities found)** 1. **Unbounded size in `receive_data` (src/shared/protocol.c:185-197)** `malloc((size_t)size)` where `size` is an `unsigned long long` received from the network. On 32-bit systems (where `size_t` is 32-bit), a large `unsigned long long` value truncates silently. While the truncated value is used consistently for both `malloc` and `receive_n_data`, this could enable memory exhaustion attacks. Consider adding a bounds check: `if ((size_t)size != size) return NULL;` and/or a `MAX_DATA_SIZE` limit. 2. **Path traversal via symlinks (src/client/scanner.c:160-167)** `entry->d_name` is used with `stat()` (not `lstat()`), which follows symlinks. A symlink pointing outside the scanned directory (e.g., `../../../etc/passwd`) would be followed and included in the chunk data. If the scanner is used with untrusted directories, this is a path traversal risk. Document this behavior or add validation. 3. **Valgrind detection reads partial `/proc/self/maps` (tests/test_utils.h:12-21)** Only reads the first 4095 bytes. If `vgpreload` appears later in the maps file (unlikely but possible), the detection produces a false negative. Consider using `getenv("LD_PRELOAD")` or valgrind.h `RUNNING_ON_VALGRIND` macro instead. 4. **`data_create` frees caller pointer on failure without documentation (src/shared/data.c:27-37)** When `malloc` for the `Data` struct fails, `data_create` calls `free(data)`, consuming the caller's buffer. This is a "pass ownership" contract that is not documented. All current callers handle this correctly, but future callers may double-free if unaware. === GENERAL REVIEW === **Severity: LOW (logic is correct; minor issues)** 1. **Overall logic correctness: PASS** - Deep-copy of exclude/include patterns correctly frees partially-allocated resources on failure. - `data_create_empty(0)` fix correctly handles `malloc(0)` UB. - `receive_str` size cap (`MAX_STRING_SIZE = 10MB`) prevents unbounded allocation. Good security fix. - `transport_ssh.c` dynamic argv array (32 slots vs hardcoded 16) with bounds checks prevents buffer overflow. - `mkdir_r` replaces `strcpy` with `memcpy` and bounds checks — eliminates buffer overflow. 2. **Thread safety concern: bwlimit state is not thread-safe (src/shared/protocol.c:15-17)** `bw_tokens` and `bw_last_refill` are global variables (not `__thread`). If multiple threads call `send_n_data`/`receive_n_data` concurrently, the bandwidth limiter state will race. Pre-existing issue, not introduced by this PR. 3. **`delete_extras_walk` all_removed logic (src/shared/utils.c:106-155)** Complex state tracking that is correct but fragile. The `rmdir` with `ENOENT` ignoring is correct, but an explanatory comment would improve maintainability. 4. **`transport_ssh.c`: no error message before `_exit(1)` on allocation failure (line 129-130)** If `calloc` for ssh_argv fails, the child process silently exits with `_exit(1)`. No error message is printed, making debugging difficult. 5. **Test quality: GOOD** - New test `test_data_create_empty_zero` covers the malloc(0) fix. - New test `test_receive_str_oversized` covers the MAX_STRING_SIZE cap. - Valgrind detection via `/proc/self/maps` replaces fragile environment variable check. === VERDICT === **PASS** with recommendations The PR correctly fixes several memory safety issues (malloc(0) UB, buffer overflow in mkdir_r, array bounds in ssh transport, missing NULL checks, unbounded string allocation). Code quality is generally good with clean style, consistent error handling, and proper resource cleanup on failure paths. The issues found are minor (unused variable, wrong printf format specifiers, DRY duplication, include style inconsistency) and do not warrant rejection. I recommend addressing them in a follow-up cleanup. Key recommendations to address: 1. Remove unused `path2_pointer` in `utils.c:168` 2. Fix `%lld` --> `%llu` in `protocol.c:181,196` 3. Fix `#include "stdlib.h"` --> `#include <stdlib.h>` in `data.c:3` 4. Add size bounds check in `receive_data` for 32-bit safety 5. Refactor scanner.c deep-copy duplication

TapTap added 1 commit 2026-07-21 14:16:11 +02:00

fix: address review findings — unused var, constness, format specifiers, bounds check 23af379bfb

TapTap commented 2026-07-21 14:36:29 +02:00

Author

Owner

Copy Link

Copy Source

Re-review: 5/6 FIXES VERIFIED, 1 NEEDS REWORK 🔶

Verified ✅

1. Unused path2_pointer removed (utils.c:158) — Variable removed, path2++ used directly ✅
2. Include style fixed (data.c:3) — #include <stdlib.h> with angle brackets ✅
3. const char* path2 (utils.c:161, utils.h:9) — Both declaration and prototype updated ✅
4. %llu format specifiers (protocol.c:181,196) — Both send_data and receive_data use %llu for unsigned long long ✅
5. lstat() for symlinks (scanner.c:162) — lstat replaces stat ✅

Not Implemented ❌

5. Bounds check in receive_data — The specific guard if ((size_t)size != size) return NULL; was not added. The function casts unsigned long long size directly to (size_t) without verifying the value fits (lines 189, 192, 197). This is a portability concern on platforms where size_t < unsigned long long (e.g., 32-bit).

   Note: A MAX_STRING_SIZE bounds check WAS added to receive_str (protocol.h:9, protocol.c:158-162), but that is a different function.

Verdict

APPROVED with one item requiring a follow-up. Fix #5 should be added to receive_data before merging, or explicitly documented as not applicable for the 64-bit-only target.

## Re-review: 5/6 FIXES VERIFIED, 1 NEEDS REWORK 🔶 ### Verified ✅ 1. **Unused `path2_pointer` removed** (utils.c:158) — Variable removed, `path2++` used directly ✅ 2. **Include style fixed** (data.c:3) — `#include <stdlib.h>` with angle brackets ✅ 3. **`const char* path2`** (utils.c:161, utils.h:9) — Both declaration and prototype updated ✅ 4. **`%llu` format specifiers** (protocol.c:181,196) — Both `send_data` and `receive_data` use `%llu` for `unsigned long long` ✅ 6. **`lstat()` for symlinks** (scanner.c:162) — `lstat` replaces `stat` ✅ ### Not Implemented ❌ 5. **Bounds check in `receive_data`** — The specific guard `if ((size_t)size != size) return NULL;` was **not added**. The function casts `unsigned long long size` directly to `(size_t)` without verifying the value fits (lines 189, 192, 197). This is a portability concern on platforms where `size_t < unsigned long long` (e.g., 32-bit). *Note: A `MAX_STRING_SIZE` bounds check WAS added to `receive_str` (protocol.h:9, protocol.c:158-162), but that is a different function.* ### Verdict **APPROVED with one item requiring a follow-up.** Fix #5 should be added to `receive_data` before merging, or explicitly documented as not applicable for the 64-bit-only target.

TapTap added 1 commit 2026-07-21 14:37:06 +02:00

fix: add MAX_DATA_SIZE bounds check in receive_data ddfd0f1cc2

TapTap added 1 commit 2026-07-21 14:39:38 +02:00

fix: const qualifier for dirent pointer (cppcheck) 94ff55f256

TapTap commented 2026-07-21 14:41:19 +02:00

Author

Owner

Copy Link

Copy Source

Re-review #2: Fixes RE-VERIFIED ✅

All fixes confirmed:

1. ✅ Unused path2_pointer removed
2. ✅ Include style fixed
3. ✅ const char* path2
4. ✅ %llu format specifiers
5. ✅ MAX_DATA_SIZE bounds check in receive_data
6. ✅ lstat() for symlinks
7. ✅ const struct dirent* entry (cppcheck)

CI: ✅ All green

Verdict: FULLY APPROVED

## Re-review #2: Fixes RE-VERIFIED ✅ All fixes confirmed: 1. ✅ Unused path2_pointer removed 2. ✅ Include style fixed 3. ✅ const char* path2 4. ✅ %llu format specifiers 5. ✅ MAX_DATA_SIZE bounds check in receive_data 6. ✅ lstat() for symlinks 7. ✅ const struct dirent* entry (cppcheck) CI: ✅ All green **Verdict: FULLY APPROVED**

TapTap referenced this issue from a commit 2026-07-21 15:11:24 +02:00

Merge all remaining PRs (#91, #92, #87, #89) into single branch

TapTap referenced this pull request 2026-07-21 15:11:28 +02:00

Merge remaining 4 PRs: memory safety, refactoring, test coverage, integration cleanup #93

TapTap closed this pull request 2026-07-21 16:10:17 +02:00

Some checks are pending

Hide all checks

CI / lint (pull_request) Successful in 7s

Details

CI / sanitizers (address) (pull_request) Successful in 14s

Details

CI / sanitizers (undefined) (pull_request) Successful in 15s

Details

CI / fuzz-build (pull_request) Successful in 13s

Details

CI / coverage (pull_request) Successful in 10s

Details

CI / valgrind (pull_request) Successful in 11s

Details

CI / build-and-test (pull_request) Successful in 54s

Details

Pull request closed

Please reopen this pull request to perform a merge.

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