GH-45497: [C++][CSV] Avoid buffer overflow when a line has too many columns by pitrou · Pull Request #45498 · apache/arrow

pitrou · 2025-02-11T14:44:46Z

What changes are included in this PR?

Add guard against going past the buffer's end, while minimizing the performance overhead of the runtime check.
Add error propagation for buffer (re)allocation, instead of aborting. This is unrelated to the reported issue, but is desirable nevertheless.

With these changes, a CSV line with an unexpectedly large number of columns will raise an error such as "CSV parse error: Expected 13 columns, got ...".

Are these changes tested?

Yes, by additional unit tests.

Are there any user-facing changes?

No.

GitHub Issue: [C++] Potential buffer overflow when CSV line contains too many columns #45497

pitrou · 2025-02-11T15:12:22Z

@github-actions crossbow submit -g cpp

pitrou · 2025-02-11T15:12:29Z

@ursabot please benchmark

ursabot · 2025-02-11T15:12:36Z

Benchmark runs are scheduled for commit 463fe8c23da28dcea4c7fae2d712cc661295a844. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

pitrou · 2025-02-11T17:37:10Z

@zanmato1984 Sorry for pinging you once again! I'm looking for reviewers on this PR :)

conbench-apache-arrow · 2025-02-11T21:55:18Z

Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 463fe8c23da28dcea4c7fae2d712cc661295a844.

There were 14 benchmark results indicating a performance regression:

Pull Request Run on arm64-t4g-2xlarge-linux at 2025-02-11 17:25:29Z
- BM_WriteColumn (C++) with params=<false,Int64Type>, source=cpp-micro, suite=parquet-arrow-reader-writer-benchmark
- WriteCsvStringWithQuote (C++) with params=10, source=cpp-micro, suite=arrow-csv-writer-benchmark
and 12 more (see the report linked below)

The full Conbench report has more details.

kou · 2025-02-12T00:18:19Z

If the Resize() failed, is values_[values_size_++] = v safe?

Hmm, that's a good point. We might adopt the same kind of approach as in the presized writer below.

kou · 2025-02-12T00:32:14Z

Why do we need 2, 5, 10 here? 1, 100 aren't enough?

The conditions for triggering the issue are bit mysterious, so I thought it would be safer to ensure non-regression with a larger set of parameters. I can trim it down a bit of course.

kou · 2025-02-12T00:34:07Z

Do we need all these cases?
It seems that -1, 1, 32767 are enough.

Same answer as above :)

mapleFU · 2025-02-12T03:41:35Z

when !status_.ok(), the values_buffer_ isn't gurantee to exist?

It will probably exist, but it might not be large enough.

zanmato1984

Other than kou's comment about the writing in ResizableValueDescWriter when Resize failed, this LGTM.

…many columns * Add guard against going past the buffer's end, while minimizing the performance overhead of the runtime check * Add error propagation for buffer (re)allocation, instead of aborting

pitrou · 2025-02-12T10:43:57Z

@github-actions crossbow submit -g cpp

pitrou · 2025-02-12T10:44:05Z

@ursabot please benchmark

ursabot · 2025-02-12T10:44:12Z

Benchmark runs are scheduled for commit 2d1965f. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

github-actions · 2025-02-12T10:46:27Z

Revision: 2d1965f

Submitted crossbow builds: ursacomputing/crossbow @ actions-4b9f129152

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

pitrou · 2025-02-12T10:50:06Z

(by the way, do we have an open issue for the failing Debian builds? @kou)

kou · 2025-02-12T10:52:35Z

GH-45510

pitrou · 2025-02-12T11:46:19Z

I think I've addressed all review comments. Does someone want to take a quick look or should I merge?

zanmato1984

Just one question.

zanmato1984 · 2025-02-12T11:52:37Z

+      }
+      status_ &= std::move(resize_status);
+    }
+    if (ARROW_PREDICT_TRUE(status_.ok())) {


Why not the same approach as PresizedValueDescWriter as you mentioned before? Less perf hurting?

As mentioned by @mapleFU above, the pointer might not be valid anymore if the resize failed.

Also, this is less performance-critical as this is normally run only on the first line of data.

I could add comments to explain this if that's useful.

As mentioned by @mapleFU above, the pointer might not be valid anymore if the resize failed.

Ah, good point.

Also, this is less performance-critical as this is normally run only on the first line of data.

I don't think that's necessary. Do it as you wish.

conbench-apache-arrow · 2025-02-12T17:07:24Z

Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 2d1965f.

There were 9 benchmark results indicating a performance regression:

Pull Request Run on amd64-c6a-4xlarge-linux at 2025-02-12 11:39:15Z
- CopyShortVector (C++) with params=<STD_VECTOR(std::string)>, source=cpp-micro, suite=arrow-small-vector-benchmark
- ArrayArrayKernel (C++) with params=<Subtract, UInt8Type>/size:524288/inverse_null_proportion:100, source=cpp-micro, suite=arrow-compute-scalar-arithmetic-benchmark
and 7 more (see the report linked below)

The full Conbench report has more details.

conbench-apache-arrow · 2025-02-12T19:42:09Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 25d8bed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 15 possible false positives for unstable benchmarks that are known to sometimes produce them.

pitrou changed the title ~~GH-45497: [C++][CSV] Avoid buffer overflow when a line has too many c…~~ GH-45497: [C++][CSV] Avoid buffer overflow when a line has too many columns Feb 11, 2025

github-actions Bot added the awaiting review Awaiting review label Feb 11, 2025

pitrou added the Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. label Feb 11, 2025

pitrou marked this pull request as ready for review February 11, 2025 15:12

This comment was marked as outdated.

Sign in to view

kou reviewed Feb 12, 2025

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Feb 12, 2025

mapleFU reviewed Feb 12, 2025

View reviewed changes

zanmato1984 reviewed Feb 12, 2025

View reviewed changes

Comment thread cpp/src/arrow/csv/parser.cc Outdated

pitrou mentioned this pull request Feb 12, 2025

[DISCUSS] Exceptions vs status codes apache/iceberg-cpp#14

Closed

zanmato1984 reviewed Feb 12, 2025

View reviewed changes

Comment thread cpp/src/arrow/csv/parser_test.cc Outdated

zanmato1984 reviewed Feb 12, 2025

View reviewed changes

pitrou added 2 commits February 12, 2025 11:21

apacheGH-45497: [C++][CSV] Avoid buffer overflow when a line has too …

05f2d4b

…many columns * Add guard against going past the buffer's end, while minimizing the performance overhead of the runtime check * Add error propagation for buffer (re)allocation, instead of aborting

Address review comments

2d1965f

pitrou force-pushed the csv-vuln branch from 463fe8c to 2d1965f Compare February 12, 2025 10:43

github-actions Bot added the Component: C++ label Feb 12, 2025

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 12, 2025

zanmato1984 reviewed Feb 12, 2025

View reviewed changes

zanmato1984 approved these changes Feb 12, 2025

View reviewed changes

Add explanatory comment

b4eb09f

pitrou merged commit 25d8bed into apache:main Feb 12, 2025

pitrou removed the awaiting change review Awaiting change review label Feb 12, 2025

pitrou deleted the csv-vuln branch February 12, 2025 12:54

pitrou mentioned this pull request Feb 12, 2025

[C++] Potential buffer overflow when CSV line contains too many columns #45497

Closed

Conversation

pitrou commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

pitrou commented Feb 11, 2025

Uh oh!

pitrou commented Feb 11, 2025

Uh oh!

ursabot commented Feb 11, 2025

Uh oh!

This comment was marked as outdated.

pitrou commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conbench-apache-arrow Bot commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Feb 12, 2025

Uh oh!

pitrou commented Feb 12, 2025

Uh oh!

ursabot commented Feb 12, 2025

Uh oh!

github-actions Bot commented Feb 12, 2025

Uh oh!

pitrou commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Feb 12, 2025

Uh oh!

pitrou commented Feb 12, 2025

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

conbench-apache-arrow Bot commented Feb 12, 2025

Uh oh!

conbench-apache-arrow Bot commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pitrou commented Feb 11, 2025 •

edited

Loading

pitrou commented Feb 11, 2025 •

edited

Loading

pitrou commented Feb 12, 2025 •

edited

Loading