Skip to content

Commit eb35215

Browse files
vincentlaucsbvincentlaucsb
authored
Version 2.4.1: CSV Parser Bulletproofing (#287)
* Set up memory sanitizers * Update sanitizers.yml * Fix poorly organized iterator test suite * Fix thread safety issue in test Thread safety issue wasn't in library, but rather a test was was not constructed properly. * Fix potential heap use after free in CSVRow::iterator * Fix CSV iterator test to not use std::max_element * Update sanitizers.yml * Made CSVField::try_parse_hex a template Allows for using long long for better overflow safety without breaking int usages * Disable gcov by default * Fix more thread safety issues Fixed with low cost copies of simple data structures to avoid possible concurrency related issues * Update thread_safe_deque.hpp * Replace std::deque with std::map in internal data structure std::deque actually does have invalidation issues (of iterators) when expanding just like std::vector * Fix issue with std::map implementation * Update thread_safe_deque.hpp Use more lockless sync. Fix some thread safety issues. * Update thread_safe_deque.hpp * Use std::vector of unique_ptrs to RawCSVFields Concurrent R/W to std::map is UB * Remove some unnecessary locks in ThreadSafeDeque
1 parent 4785edf commit eb35215

20 files changed

Lines changed: 1223 additions & 386 deletions

‎.claude/rules/csv_reader_rules.md‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
---
2+
paths:
3+
- "include/internal/csv_reader.hpp"
4+
- "include/internal/csv_reader.cpp"
5+
- "include/internal/csv_reader_iterator.cpp"
6+
---
7+
8+
# CSVReader Iterator Rules
9+
10+
## CRITICAL CONSTRAINT: DO NOT Cache All RawCSVDataPtr Chunks
11+
12+
### The Streaming Architecture
13+
- Library parses 50+ GB CSV files with bounded memory (e.g., 8GB RAM)
14+
- Previous data chunks are freed as iterator advances
15+
- `CSVReader::iterator` is `std::input_iterator_tag` by design (single-pass)
16+
17+
### When You See Heap-Use-After-Free with std::max_element or ForwardIterator Algorithms
18+
19+
**❌ WRONG FIX (defeats streaming)**:
20+
```cpp
21+
class iterator {
22+
std::vector<RawCSVDataPtr> _all_data_chunks; // NO!
23+
// Result: 50GB CSV requires 50GB RAM
24+
};
25+
```
26+
27+
**✅ CORRECT FIXES**:
28+
1. **Document limitation**: ForwardIterator algorithms not supported on CSVReader::iterator
29+
2. **Fix test**: Copy to vector first: `std::vector<CSVRow> rows(reader.begin(), reader.end());`
30+
3. **Update README**: Show vector-based workaround for std::max_element
31+
32+
### Valid RawCSVDataPtr Usage
33+
- ✅ `CSVRow::iterator` CAN have RawCSVDataPtr member (single row scope, negligible memory)
34+
- ❌ `CSVReader::iterator` CANNOT cache all chunks (file scope, unbounded memory)
35+
36+
### Root Cause Analysis
37+
- Small test files (< chunk size) fit in one chunk → ForwardIterator algorithms appear to work
38+
- Large files span multiple chunks → heap-use-after-free when algorithm accesses freed chunk
39+
- Solution: Document as unsupported OR provide vector workaround, NOT cache all chunks

‎.github/workflows/README.md‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
# GitHub Actions Memory Checking Setup
2+
3+
This directory contains GitHub Actions workflows for comprehensive memory and thread safety testing.
4+
5+
## Workflows
6+
7+
### 🧵 sanitizers.yml - Memory & Thread Sanitizers (PRIMARY)
8+
**Runs:** On every push and pull request
9+
**Sanitizers Included:**
10+
- **ThreadSanitizer (TSan)** - Detects data races and thread safety issues (CRITICAL for csv-parser)
11+
- **AddressSanitizer (ASan)** - Detects memory errors: use-after-free, buffer overflows, memory leaks
12+
- **UndefinedBehaviorSanitizer (UBSan)** - Catches undefined behavior: signed overflow, type mismatches
13+
14+
**Config:**
15+
- Runs on Ubuntu with GCC
16+
- Tests C++17 and C++20 standards
17+
- Debug builds for better diagnostics
18+
- Timeout: 20 minutes per configuration
19+
- Artifacts: Upload logs on failure
20+
21+
**Key Features:**
22+
- Matrix testing: 3 sanitizers × 2 C++ standards = 6 parallel jobs
23+
- Fail-fast disabled to see all results
24+
- Environment variables configured for halt-on-error behavior
25+
26+
### 💾 valgrind.yml - Valgrind Memory Profiler
27+
**Runs:** On every push and pull request
28+
**Uses:** Valgrind with full leak checking
29+
30+
**Config:**
31+
- Runs on Ubuntu with GCC
32+
- Debug build with -O1 for balance
33+
- Excludes multi-threaded tests (Valgrind + threads = slow)
34+
- Timeout: 60 minutes (Valgrind is slower)
35+
36+
### 🔍 codeql.yml - Static Analysis (GitHub CodeQL)
37+
**Runs:** On every push and pull request + weekly schedule
38+
**Analysis:**
39+
- Deep static analysis for security and quality issues
40+
- Integrates with GitHub Security tab
41+
42+
## Testing Recommendations
43+
44+
### Local Testing Before Push
45+
```bash
46+
# Test with ThreadSanitizer (most critical)
47+
cmake -B build/tsan -DCMAKE_BUILD_TYPE=Debug \
48+
-DCMAKE_CXX_FLAGS="-fsanitize=thread -g"
49+
cmake --build build/tsan
50+
cd build/tsan && ctest --output-on-failure && cd ../..
51+
52+
# Test with AddressSanitizer
53+
cmake -B build/asan -DCMAKE_BUILD_TYPE=Debug \
54+
-DCMAKE_CXX_FLAGS="-fsanitize=address -fno-omit-frame-pointer -g"
55+
cmake --build build/asan
56+
cd build/asan && ctest --output-on-failure && cd ../..
57+
```
58+
59+
### CI/CD Pipeline Order
60+
1. **Primary:** ThreadSanitizer - Catches data races in threading model
61+
2. **Primary:** AddressSanitizer - Catches memory corruption
62+
3. **Secondary:** UBSanitizer - Catches subtle undefined behavior
63+
4. **Nightly:** Valgrind - Comprehensive memory profiling
64+
5. **Continuous:** CodeQL - Static analysis
65+
66+
## Known Considerations
67+
68+
### ThreadSanitizer (TSan)
69+
- **Why CRITICAL:** csv-parser uses worker threads + ThreadSafeDeque + double_quote_fields lazy init
70+
- May report false positives in STL (benign race detection)
71+
- Slower execution due to extra instrumentation
72+
- Catches real issues in PR #282 exception propagation and issue #278 move semantics
73+
74+
### AddressSanitizer (ASan)
75+
- **Why Important:** Catches CSVFieldList memory issues like issue #278
76+
- Cannot run simultaneously with TSan (different memory models)
77+
- Better performance than TSan for memory safety
78+
79+
### Valgrind
80+
- Slower than sanitizers but more mature tool
81+
- Useful for final verification before releases
82+
- Runs on push (not PR) to avoid GitHub Actions timeout
83+
84+
### CodeQL
85+
- No runtime overhead
86+
- Integrated into GitHub Security tab
87+
- Good for catching security issues and code quality
88+
89+
## Interpreting Results
90+
91+
### Sanitizer Failures
92+
Look for:
93+
```
94+
ERROR: ThreadSanitizer: data race detected
95+
ERROR: AddressSanitizer: heap-buffer-overflow
96+
ERROR: UndefinedBehaviorSanitizer: signed integer overflow
97+
```
98+
99+
### Action Items
100+
1. Check artifacts uploaded on test failure
101+
2. Look at the specific sanitizer output in GitHub Actions logs
102+
3. Focus on ThreadSanitizer results first (threading complexity)
103+
4. Correlate with codebase changes to identify root cause
104+
105+
## History
106+
107+
- **PR #282:** ThreadSanitizer would catch exception propagation issues
108+
- **Issue #278:** AddressSanitizer catches CSVFieldList move semantics bug
109+
- **PR #237:** TSan catches double_quote_fields concurrency todo
110+
- **v2.3.0:** ThreadSafeDeque prevents std::vector reallocation race
111+
112+
## Reference
113+
114+
- [ThreadSanitizer Documentation](https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual)
115+
- [AddressSanitizer Documentation](https://github.com/google/sanitizers/wiki/AddressSanitizer)
116+
- [Valgrind User Manual](https://valgrind.org/docs/manual/)
117+
- [CodeQL Query Help](https://codeql.github.io/codeql-query-help/)

‎.github/workflows/cmake-multi-platform.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ name: CMake on multiple platforms
44

55
on:
66
push:
7-
branches: [ "master", "memory-fix-csvfieldlist" ]
7+
branches: [ "master", "feb-15-2026" ]
88
pull_request:
99
branches: [ "master" ]
1010

‎.github/workflows/codeql.yml‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
name: CodeQL Analysis
2+
3+
on:
4+
push:
5+
branches: [ "master", "feb-15-2026" ]
6+
pull_request:
7+
branches: [ "master" ]
8+
schedule:
9+
- cron: '0 2 * * 0' # Weekly on Sunday at 2 AM UTC
10+
11+
jobs:
12+
analyze:
13+
name: CodeQL C++
14+
runs-on: ubuntu-latest
15+
permissions:
16+
actions: read
17+
contents: read
18+
security-events: write
19+
20+
strategy:
21+
fail-fast: false
22+
matrix:
23+
language: ['cpp']
24+
25+
steps:
26+
- name: Checkout repository and submodules
27+
uses: actions/checkout@v4
28+
with:
29+
submodules: recursive
30+
31+
- name: Initialize CodeQL
32+
uses: github/codeql-action/init@v3
33+
with:
34+
language: ${{ matrix.language }}
35+
queries: security-and-quality
36+
37+
- name: Install dependencies
38+
run: |
39+
sudo apt-get update
40+
sudo apt-get install -y build-essential cmake
41+
42+
- name: Configure CMake
43+
run: |
44+
mkdir -p build
45+
cd build
46+
cmake .. \
47+
-DCMAKE_BUILD_TYPE=Release \
48+
-DCSV_CXX_STANDARD=17 \
49+
-DCMAKE_CXX_COMPILER=g++ \
50+
-DCMAKE_C_COMPILER=gcc
51+
52+
- name: Build for CodeQL analysis
53+
run: cmake --build build --config Release
54+
55+
- name: Run CodeQL analysis
56+
uses: github/codeql-action/analyze@v3
57+
with:
58+
category: "/language:${{ matrix.language }}"

‎.github/workflows/sanitizers.yml‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
name: Memory and Thread Sanitizers
2+
3+
on:
4+
push:
5+
branches: [ "master", "feb-15-2026" ]
6+
pull_request:
7+
branches: [ "master" ]
8+
9+
jobs:
10+
sanitizers:
11+
runs-on: ubuntu-latest
12+
13+
strategy:
14+
fail-fast: false
15+
matrix:
16+
sanitizer: [address, thread, undefined]
17+
cxx_standard: [17, 20]
18+
include:
19+
- sanitizer: address
20+
flag: "-fsanitize=address -fno-omit-frame-pointer"
21+
name: "AddressSanitizer"
22+
- sanitizer: thread
23+
flag: "-fsanitize=thread"
24+
name: "ThreadSanitizer"
25+
- sanitizer: undefined
26+
flag: "-fsanitize=undefined -fno-omit-frame-pointer"
27+
name: "UndefinedBehaviorSanitizer"
28+
29+
env:
30+
ASAN_OPTIONS: "detect_leaks=1:halt_on_error=0"
31+
TSAN_OPTIONS: "halt_on_error=1"
32+
UBSAN_OPTIONS: "halt_on_error=1"
33+
34+
steps:
35+
- name: Checkout repository and submodules
36+
uses: actions/checkout@v4
37+
with:
38+
submodules: recursive
39+
40+
- name: Install dependencies
41+
run: |
42+
sudo apt-get update
43+
sudo apt-get install -y build-essential cmake
44+
45+
- name: Configure CMake with ${{ matrix.name }}
46+
run: |
47+
mkdir -p build
48+
cd build
49+
cmake .. \
50+
-DCMAKE_BUILD_TYPE=Debug \
51+
-DCSV_CXX_STANDARD=${{ matrix.cxx_standard }} \
52+
-DCMAKE_CXX_COMPILER=g++ \
53+
-DCMAKE_C_COMPILER=gcc \
54+
-DCMAKE_CXX_FLAGS="${{ matrix.flag }} -g" \
55+
-DCMAKE_C_FLAGS="${{ matrix.flag }} -g"
56+
57+
- name: Build with ${{ matrix.name }}
58+
run: cmake --build build --config Debug
59+
60+
- name: Test with ${{ matrix.name }}
61+
working-directory: build
62+
run: ctest --output-on-failure -V
63+
timeout-minutes: 20
64+
65+
- name: Upload sanitizer logs
66+
if: failure()
67+
uses: actions/upload-artifact@v4
68+
with:
69+
name: sanitizer-logs-${{ matrix.sanitizer }}-std${{ matrix.cxx_standard }}
70+
path: build/Testing/
71+
72+
valgrind:
73+
runs-on: ubuntu-latest
74+
75+
steps:
76+
- name: Checkout repository and submodules
77+
uses: actions/checkout@v4
78+
with:
79+
submodules: recursive
80+
81+
- name: Install dependencies
82+
run: |
83+
sudo apt-get update
84+
sudo apt-get install -y build-essential cmake valgrind
85+
86+
- name: Configure CMake
87+
run: |
88+
mkdir -p build
89+
cd build
90+
cmake .. \
91+
-DCMAKE_BUILD_TYPE=Debug \
92+
-DCSV_CXX_STANDARD=17 \
93+
-DCMAKE_CXX_COMPILER=g++ \
94+
-DCMAKE_C_COMPILER=gcc \
95+
-DCMAKE_CXX_FLAGS="-g -O1" \
96+
-DCMAKE_C_FLAGS="-g -O1"
97+
98+
- name: Build
99+
run: cmake --build build --config Debug
100+
101+
- name: Test with Valgrind
102+
working-directory: build
103+
run: ctest --output-on-failure
104+
timeout-minutes: 60
105+
106+
- name: Upload Valgrind results
107+
if: failure()
108+
uses: actions/upload-artifact@v4
109+
with:
110+
name: valgrind-results
111+
path: build/Testing/

‎CMakeLists.txt‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ else()
88
endif(CSV_CXX_STANDARD)
99

1010
option(BUILD_PYTHON "Build Python Binding" OFF)
11+
option(ENABLE_CODE_COVERAGE "Enable code coverage instrumentation" OFF)
1112

1213
message("Building CSV library using C++${CMAKE_CXX_STANDARD}")
1314

@@ -29,7 +30,11 @@ if(MSVC)
2930
else()
3031
# Ignore Visual Studio pragma regions
3132
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas")
32-
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} --coverage -Og")
33+
34+
if(ENABLE_CODE_COVERAGE)
35+
message("Code coverage instrumentation enabled")
36+
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} --coverage -Og")
37+
endif()
3338
endif(MSVC)
3439

3540
set(CSV_ROOT_DIR ${CMAKE_CURRENT_LIST_DIR})

0 commit comments

Comments
 (0)