Skip to content

Commit c6f6f5b

Browse files
committed
Harden parser lifetime, EOF handling, and validation tooling
Keep the existing streaming C++11 parser API and permissive CSV behavior while tightening correctness and production validation. Parser changes:\n- Fix EOF handling for trailing empty fields, empty quoted fields, and empty fields at EOF.\n- Fix position() accounting so it tracks consumed parser position instead of buffered chunk size.\n- Add an owning std::unique_ptr<std::istream> constructor and CsvParser::from_file(path).\n- Preserve the borrowed std::istream& constructor for compatibility.\n- Reject null owned streams and failed input streams early.\n- Improve common payload parsing by appending contiguous field spans instead of pushing every byte individually. Validation and release tooling:\n- Add regression tests for EOF, position, and stream ownership behavior.\n- Add RapidCheck property tests.\n- Add AFL-style, libFuzzer, and deterministic sanitizer mutation fuzz harnesses.\n- Add GitHub Actions CI for tests, fuzz smoke builds, and benchmark harness checks.\n- Add security notes and benchmark/profiling workflow docs. Compatibility:\n- Existing CsvParser(std::istream&) usage continues to work.\n- Default parsing remains permissive.\n- No strict mode or size limits are enabled by default.
1 parent 43961a9 commit c6f6f5b

19 files changed

Lines changed: 1177 additions & 16 deletions

.github/workflows/ci.yml

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
name: CI
2+
3+
on:
4+
push:
5+
pull_request:
6+
7+
jobs:
8+
test:
9+
runs-on: ubuntu-latest
10+
steps:
11+
- uses: actions/checkout@v4
12+
13+
- name: Configure unit tests
14+
run: cmake -S test -B test/out
15+
16+
- name: Build unit tests
17+
run: cmake --build test/out --parallel
18+
19+
- name: Run unit tests
20+
run: ./test/out/parser_test
21+
22+
- name: Configure property tests
23+
run: cmake -S test -B test/property-out -DARIA_CSV_ENABLE_PROPERTY_TESTS=ON
24+
25+
- name: Build property tests
26+
run: cmake --build test/property-out --parallel
27+
28+
- name: Run property tests
29+
run: ./test/property-out/parser_property_test
30+
31+
fuzz-smoke:
32+
runs-on: ubuntu-latest
33+
steps:
34+
- uses: actions/checkout@v4
35+
36+
- name: Build AFL-compatible harness
37+
run: clang++ -std=c++11 -Wall -Wextra -pedantic -I. fuzz/afl_parser.cpp -o /tmp/aria_csv_afl_parser
38+
39+
- name: Build libFuzzer harness
40+
run: clang++ -std=c++11 -Wall -Wextra -pedantic -I. -fsanitize=fuzzer,address,undefined fuzz/libfuzzer_parser.cpp -o /tmp/aria_csv_libfuzzer
41+
42+
- name: Configure CMake fuzz targets
43+
run: cmake -S . -B /tmp/aria_csv_fuzz_build -DARIA_CSV_BUILD_FUZZERS=ON
44+
45+
- name: Build CMake AFL-compatible fuzz target
46+
run: cmake --build /tmp/aria_csv_fuzz_build --target aria_csv_afl_fuzzer --parallel
47+
48+
- name: Run libFuzzer smoke test
49+
run: |
50+
mkdir -p /tmp/aria_csv_corpus
51+
cp test/data/*.csv /tmp/aria_csv_corpus/
52+
/tmp/aria_csv_libfuzzer /tmp/aria_csv_corpus -runs=1000
53+
54+
build:
55+
runs-on: ubuntu-latest
56+
steps:
57+
- uses: actions/checkout@v4
58+
59+
- name: Build benchmark harnesses
60+
run: |
61+
clang++ -std=c++11 -Wall -Wextra -pedantic -I. -c benchmark/cpp/main.cpp -o /tmp/aria_csv_benchmark.o
62+
clang++ -std=c++11 -Wall -Wextra -pedantic -I. -c benchmark/profile.cpp -o /tmp/aria_csv_profile.o
63+
clang++ -std=c++11 -Wall -Wextra -pedantic -I. -c benchmark/optimize.cpp -o /tmp/aria_csv_optimize.o
64+
65+
- name: Run optimization harness smoke test
66+
run: |
67+
clang++ -std=c++11 -O2 -DNDEBUG -I. benchmark/optimize.cpp -o /tmp/aria_csv_optimize
68+
/tmp/aria_csv_optimize 1 | tee /tmp/aria_csv_optimize_a.csv
69+
/tmp/aria_csv_optimize 1 | tee /tmp/aria_csv_optimize_b.csv
70+
python3 benchmark/compare_results.py /tmp/aria_csv_optimize_a.csv /tmp/aria_csv_optimize_b.csv

CMakeLists.txt

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
1-
project(AriaCsvParser)
21
cmake_minimum_required(VERSION 3.14 FATAL_ERROR)
2+
project(AriaCsvParser)
33
include(GNUInstallDirs)
44
include(CMakePackageConfigHelpers)
55

6+
option(ARIA_CSV_BUILD_FUZZERS "Build parser fuzz targets" OFF)
7+
option(ARIA_CSV_BUILD_LIBFUZZER "Build the libFuzzer parser target" OFF)
8+
69
add_library(${PROJECT_NAME} INTERFACE)
710

811
set_target_properties(${PROJECT_NAME} PROPERTIES
@@ -16,7 +19,8 @@ target_include_directories( ${PROJECT_NAME}
1619
)
1720

1821
install(TARGETS ${PROJECT_NAME}
19-
EXPORT ${PROJECT_NAME}Targets)
22+
EXPORT ${PROJECT_NAME}Targets
23+
PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
2024

2125
install(EXPORT ${PROJECT_NAME}Targets
2226
NAMESPACE AriaCsvParser::
@@ -34,4 +38,8 @@ configure_package_config_file(Config.cmake.in
3438
install(FILES
3539
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake
3640
DESTINATION lib/cmake/${PROJECT_NAME}
37-
)
41+
)
42+
43+
if(ARIA_CSV_BUILD_FUZZERS)
44+
add_subdirectory(fuzz)
45+
endif()

README.md

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,23 @@ or you can read from `stdin`
2020
CsvParser parser(std::cin);
2121
```
2222

23+
The parser can also own the input stream, which is safer when the parser needs
24+
to outlive the scope where the stream is created.
25+
26+
```cpp
27+
auto parser = CsvParser::from_file("some_file.csv");
28+
```
29+
30+
or
31+
32+
```cpp
33+
std::unique_ptr<std::istream> input(new std::ifstream("some_file.csv"));
34+
CsvParser parser(std::move(input));
35+
```
36+
37+
When using the `std::istream&` constructor, the caller must keep the stream alive
38+
for at least as long as the parser.
39+
2340
Moreover, you can configure the parser by chaining configuration methods like
2441
2542
```cpp
@@ -72,7 +89,7 @@ int main() {
7289
auto field = parser.next_field();
7390
switch (field.type) {
7491
case FieldType::DATA:
75-
std::cout << *field.data << " | ";
92+
std::cout << field.data << " | ";
7693
break;
7794
case FieldType::ROW_END:
7895
std::cout << std::endl;
@@ -92,4 +109,21 @@ reporting things like progress through a file. You can use
92109

93110
## Testing
94111

95-
Run `cmake -B out && cmake --build out && ./out/parser_test` in test dir
112+
Run the unit tests with:
113+
114+
```sh
115+
cmake -S test -B test/out
116+
cmake --build test/out
117+
./test/out/parser_test
118+
```
119+
120+
Property tests are opt-in and use RapidCheck:
121+
122+
```sh
123+
cmake -S test -B test/out -DARIA_CSV_ENABLE_PROPERTY_TESTS=ON
124+
cmake --build test/out
125+
./test/out/parser_property_test
126+
```
127+
128+
Fuzz targets live in `fuzz/`. See `fuzz/README.md` for libFuzzer and AFL++
129+
commands.

SECURITY.md

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
# Security Notes
2+
3+
This parser is intended to be small, streaming, and dependency-free. It now
4+
supports both borrowed and owned input streams.
5+
6+
## Stream Lifetime
7+
8+
`CsvParser(std::istream &)` borrows the stream. The caller must keep that stream
9+
alive for the full lifetime of the parser.
10+
11+
For stored parsers or parser objects that outlive a local stream variable, use an
12+
owned stream:
13+
14+
```cpp
15+
auto parser = aria::csv::CsvParser::from_file("data.csv");
16+
```
17+
18+
or:
19+
20+
```cpp
21+
std::unique_ptr<std::istream> input(new std::ifstream("data.csv"));
22+
aria::csv::CsvParser parser(std::move(input));
23+
```
24+
25+
The owning constructor rejects null streams and failed streams.
26+
27+
## Untrusted Input
28+
29+
The parser is permissive by design. It does not currently reject malformed CSV
30+
such as unclosed quotes. For untrusted data pipelines that require validation,
31+
add a strict parsing layer or a future strict mode rather than relying on
32+
permissive parsing as validation.
33+
34+
The parser stores the current field in memory. Very large fields can therefore
35+
consume large amounts of memory. Applications that accept untrusted CSV should
36+
enforce input-size limits at the application or stream layer.
37+
38+
## Fuzzing
39+
40+
Fuzz harnesses are available under `fuzz/` for libFuzzer and AFL++ style runs.
41+
Use them with ASan/UBSan when changing parser control flow.

benchmark/OPTIMIZATION.md

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
# Optimization Workflow
2+
3+
The goal is to make the parser faster without letting the implementation turn
4+
into clever, brittle code. Every optimization should carry both a speed result
5+
and a readability/maintenance result.
6+
7+
## Baseline
8+
9+
Build the optimization harness with release flags:
10+
11+
```sh
12+
clang++ -std=c++11 -O2 -DNDEBUG -g -fno-omit-frame-pointer \
13+
benchmark/optimize.cpp -I. -o /tmp/aria_csv_optimize
14+
```
15+
16+
Run a baseline:
17+
18+
```sh
19+
/tmp/aria_csv_optimize 7 | tee benchmark/baseline.csv
20+
```
21+
22+
The harness prints CSV:
23+
24+
```text
25+
workload,mode,bytes,iterations,best_ms,mb_per_s,checksum
26+
```
27+
28+
Use the best time across iterations to reduce scheduler noise. Re-run the
29+
baseline before comparing a change if the machine load changed noticeably.
30+
31+
## Workloads
32+
33+
The harness generates four in-memory workloads:
34+
35+
- `plain`: common unquoted rows with many ordinary fields.
36+
- `quoted`: quoted fields, escaped quotes, commas, and embedded newlines.
37+
- `wide`: many small columns per row.
38+
- `huge-fields`: fields larger than the parser's input buffer.
39+
40+
Each workload runs through both public APIs:
41+
42+
- `fields`: direct `next_field()` parsing.
43+
- `rows`: range iteration over rows.
44+
45+
## Change Gate
46+
47+
For each optimization, record:
48+
49+
- the exact code change
50+
- baseline and after numbers
51+
- percent speed change per workload
52+
- test/fuzz/property status
53+
- readability score
54+
- decision: keep, revise, or revert
55+
56+
Suggested readability score:
57+
58+
- `1`: simpler than before
59+
- `2`: same complexity
60+
- `3`: slightly more complex but local and well named
61+
- `4`: noticeably harder to maintain
62+
- `5`: too clever for this project
63+
64+
Default policy: keep only changes that improve at least one target workload by
65+
5% or more, do not regress any major workload by more than 2%, and score `3` or
66+
better on readability.
67+
68+
Compare two runs with:
69+
70+
```sh
71+
python3 benchmark/compare_results.py benchmark/baseline.csv benchmark/after.csv
72+
```
73+
74+
The comparison script also checks that workload checksums did not change. A
75+
checksum change means the parser behavior changed or the benchmark changed, so
76+
the result is not a pure performance comparison.
77+
78+
## Verification
79+
80+
Before keeping a performance change, run:
81+
82+
```sh
83+
cmake -S test -B /tmp/aria_csv_test
84+
cmake --build /tmp/aria_csv_test --parallel
85+
/tmp/aria_csv_test/parser_test
86+
87+
cmake -S test -B /tmp/aria_csv_property -DARIA_CSV_ENABLE_PROPERTY_TESTS=ON
88+
cmake --build /tmp/aria_csv_property --parallel
89+
/tmp/aria_csv_property/parser_property_test
90+
91+
clang++ -std=c++11 -Wall -Wextra -pedantic -I. \
92+
-fsanitize=fuzzer-no-link,address,undefined \
93+
-c fuzz/libfuzzer_parser.cpp -o /tmp/aria_csv_libfuzzer.o
94+
```
95+
96+
Run the sampling profiler when a change meaningfully moves benchmark numbers:
97+
98+
```sh
99+
clang++ -std=c++11 -O2 -g -fno-omit-frame-pointer \
100+
benchmark/profile.cpp -I. -o /tmp/aria_csv_profile
101+
102+
/tmp/aria_csv_profile benchmark/sample.csv 8000 &
103+
pid=$!
104+
sample "$pid" 3 1 -file /tmp/aria_csv_profile.sample.txt
105+
wait "$pid"
106+
```
107+
108+
## Decision Record Template
109+
110+
```md
111+
## YYYY-MM-DD: <change name>
112+
113+
Hypothesis:
114+
115+
Change:
116+
117+
Benchmark:
118+
119+
| workload | mode | baseline MB/s | after MB/s | delta |
120+
| --- | --- | ---: | ---: | ---: |
121+
122+
Verification:
123+
124+
Readability score:
125+
126+
Decision:
127+
```

benchmark/PROFILING.md

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
# Profiling
2+
3+
The profiling driver keeps file I/O out of the hot path by reading the CSV once
4+
and reparsing the in-memory contents repeatedly. Build it with frame pointers and
5+
debug symbols so sampling profilers can produce useful call trees.
6+
7+
## macOS sample
8+
9+
```sh
10+
clang++ -std=c++11 -O2 -g -fno-omit-frame-pointer \
11+
benchmark/profile.cpp -I. -o /tmp/aria_csv_profile
12+
13+
/tmp/aria_csv_profile benchmark/sample.csv 100000 &
14+
pid=$!
15+
sample "$pid" 5 1 -file /tmp/aria_csv_profile.sample.txt
16+
wait "$pid"
17+
```
18+
19+
The `sample` output is a call-tree profile that can be folded into flamegraph
20+
input with stackcollapse tools, or inspected directly for the hottest parser
21+
frames.
22+
23+
## Linux perf
24+
25+
```sh
26+
clang++ -std=c++11 -O2 -g -fno-omit-frame-pointer \
27+
benchmark/profile.cpp -I. -o /tmp/aria_csv_profile
28+
29+
perf record -F 997 -g -- /tmp/aria_csv_profile benchmark/sample.csv 100000
30+
perf report
31+
```
32+
33+
For SVG flamegraphs, run Brendan Gregg's `stackcollapse-perf.pl` and
34+
`flamegraph.pl` on `perf script` output.

0 commit comments

Comments
 (0)