GH-41321: [C++][Parquet] More strict Parquet level checking #41346

mapleFU · 2024-04-23T06:27:15Z

Rationale for this change

In #41321 , user reports a corrupt when reading from a corrupt parquet file. This is because we lost some checking. Current code works on reading a normal parquet file. But when reading a corrupt file, this need to be more strict.

Currently this patch just enhance the checking on Parquet Level, the correspond value check would be add in later patches

What changes are included in this PR?

More strict parquet checkings on Level

Are these changes tested?

Already exists test, maybe we can introduce parquet file as test file

Are there any user-facing changes?

More strict checkings

GitHub Issue: [C++][Parquet] Crash / heap-use-after-free in ByteArrayChunkedRecordReader::ReadValuesSpaced() on a corrupted Parquet file #41321

mapleFU · 2024-04-23T14:19:37Z

cc @emkornfield @pitrou Would you mind take a look? This patch checks the counting is right in RecordReader

cpp/src/parquet/column_reader.cc

pitrou · 2024-05-02T14:11:51Z

cpp/src/parquet/column_reader.cc

@@ -1028,12 +1030,17 @@ class TypedColumnReaderImpl : public TypedColumnReader<DType>,
  // and number of values to read. This function is called before reading values.
  void ReadLevels(int64_t batch_size, int16_t* def_levels, int16_t* rep_levels,
                  int64_t* num_def_levels, int64_t* values_to_read) {


Is it useful to have a int64_t* num_def_levels if it's always going to be equal to batch_size? Can we remove this argument?

batch_size = std::min(batch_size, this->available_values_current_page());

The batch_size would be adjusted by the code here.

pitrou · 2024-05-02T14:12:43Z

cpp/src/parquet/column_reader.cc

  *values_read = this->ReadValues(values_to_read, values);
+  ARROW_DCHECK_GE(values_to_read, *values_read);


Can this occur on a corrupted file? If so, it should be an exception.

Yeah, you're right, this can occur on a corrupt file!

Oh-no, we cannot...If values_read is too less, it might be a corrupt file, but when values_to_read < *values_read, ReadValues returns too many...

rouault · 2024-05-07T15:21:26Z

@mapleFU Gentle ping if this PR could be moved forward. This would help me making progress on fuzzing my own code, which is now stopped by crashes in libarrow/libparquet.

pitrou · 2024-05-07T15:24:49Z

cpp/src/parquet/column_reader.cc

@@ -1045,6 +1052,7 @@ class TypedColumnReaderImpl : public TypedColumnReader<DType>,

    // Not present for non-repeated fields
    if (this->max_rep_level_ > 0 && rep_levels != nullptr) {
+      DCHECK_GT(this->max_rep_level_, 0);
      int64_t num_rep_levels = this->ReadRepetitionLevels(batch_size, rep_levels);
      if (def_levels != nullptr && *num_def_levels != num_rep_levels) {


Shouldn't this be num_rep_levels != batch_size instead?

Actually it can 🤔

emmm batch_size is being limit here, not check that

mapleFU · 2024-05-07T15:30:23Z

Sorrying for delay I just back from holidays, will address the comments tomorrow.

mapleFU · 2024-05-08T07:13:35Z

Sigh, adding checks is easy, but maybe reasoning them would be a little tricky, and 77fc23f adds some ad-hoc checkings. Not ad-hoc checkings are written everywhere in our system. Maybe I can summarize them as:

Levels Count / num_values not matches the num_values() in Page Header. LevelDecoder currently doesn't checks this 😭. But we will check the decoded def-level should equal to rep-level, and we can check available_values_current_page()
storing value not matches the level, like in ReadBatch, we parsed that we have 10 non-null values, but only reads 5. This should be regard as "value not matches level"

cpp/src/parquet/column_reader_test.cc

mapleFU · 2024-05-08T09:22:37Z

cpp/src/parquet/column_reader.cc

          }
-        } else if (this->max_def_level_ > 0) {


This branch is useless...

cpp/src/parquet/column_reader.cc

mapleFU · 2024-05-17T14:39:24Z

@github-actions crossbow submit -g cpp

github-actions · 2024-05-17T14:41:58Z

Revision: 3d58afe

Submitted crossbow builds: ursacomputing/crossbow @ actions-c74132eedf

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
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-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
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-gcc-14

mapleFU · 2024-05-20T11:10:32Z

Will merge in one day if no negative comments, since we collect 1 +1 vote

conbench-apache-arrow · 2024-05-21T15:36:10Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 1f07404.

There were no benchmark performance regressions. 🎉

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

…ache#41346) ### Rationale for this change In apache#41321 , user reports a corrupt when reading from a corrupt parquet file. This is because we lost some checking. Current code works on reading a normal parquet file. But when reading a corrupt file, this need to be more strict. **Currently this patch just enhance the checking on Parquet Level, the correspond value check would be add in later patches** ### What changes are included in this PR? More strict parquet checkings on Level ### Are these changes tested? Already exists test, maybe we can introduce parquet file as test file ### Are there any user-facing changes? More strict checkings * GitHub Issue: apache#41321 Lead-authored-by: mwish <[email protected]> Co-authored-by: mwish <[email protected]> Signed-off-by: mwish <[email protected]>

Update level and batch-read checkings

31d628a

mapleFU requested a review from wgtmac as a code owner April 23, 2024 06:27

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Apr 23, 2024

Revert changes in column-reader (since it's different from RecordReader)

7df0415

mapleFU mentioned this pull request Apr 24, 2024

[C++] [Parquet] Crash / heap-buffer-overflow in TableBatchReader::ReadNext() on a corrupted Parquet file #41317

Closed

emkornfield reviewed Apr 29, 2024

View reviewed changes

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 29, 2024

emkornfield reviewed Apr 29, 2024

View reviewed changes

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

emkornfield reviewed Apr 29, 2024

View reviewed changes

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

pitrou reviewed May 2, 2024

View reviewed changes

pitrou reviewed May 7, 2024

View reviewed changes

Refactor handling for error

4930aae

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 8, 2024

tweaks

2cf9591

mapleFU force-pushed the more-checks-for-read-levels branch from 0f49c8d to 2cf9591 Compare May 8, 2024 08:31

mapleFU changed the title ~~GH-41321: [C++][Parquet] More strict Parquet value number checking~~ GH-41321: [C++][Parquet] More strict Parquet level checking May 8, 2024

mapleFU commented May 8, 2024

View reviewed changes

cpp/src/parquet/column_reader_test.cc Outdated Show resolved Hide resolved

mapleFU commented May 8, 2024

View reviewed changes

cpp/src/parquet/column_reader.cc

}

} else if (this->max_def_level_ > 0) {

Copy link

Member Author

mapleFU May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is useless...

mapleFU requested review from pitrou and emkornfield May 8, 2024 09:23

Fix test and add more tests

db35d8b

mapleFU force-pushed the more-checks-for-read-levels branch from fbafc33 to db35d8b Compare May 8, 2024 09:24

mapleFU force-pushed the more-checks-for-read-levels branch 3 times, most recently from 70f1b66 to b08803a Compare May 16, 2024 16:46

revert back parquet-testing change

f50a393

mapleFU force-pushed the more-checks-for-read-levels branch from b08803a to f50a393 Compare May 16, 2024 16:52

mapleFU requested a review from pitrou May 16, 2024 16:52

pitrou reviewed May 16, 2024

View reviewed changes

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

pitrou reviewed May 16, 2024

View reviewed changes

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

pitrou reviewed May 16, 2024

View reviewed changes

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

pitrou reviewed May 16, 2024

View reviewed changes

cpp/src/parquet/column_reader.cc Outdated Show resolved Hide resolved

update impl

3d58afe

mapleFU requested a review from pitrou May 16, 2024 18:00

emkornfield approved these changes May 17, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 17, 2024

mapleFU mentioned this pull request May 17, 2024

[C++][Parquet][CI] DatasetEncryptionTest.WriteReadDatasetWithEncryption failed #41710

Closed

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels May 17, 2024

resolve comment

5e46d61

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 17, 2024

mapleFU merged commit 1f07404 into apache:main May 21, 2024
32 of 33 checks passed

mapleFU removed the awaiting change review Awaiting change review label May 21, 2024

mapleFU mentioned this pull request May 21, 2024

[C++][Parquet] Crash / heap-use-after-free in ByteArrayChunkedRecordReader::ReadValuesSpaced() on a corrupted Parquet file #41321

Closed

mapleFU deleted the more-checks-for-read-levels branch May 21, 2024 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41321: [C++][Parquet] More strict Parquet level checking #41346

GH-41321: [C++][Parquet] More strict Parquet level checking #41346

mapleFU commented Apr 23, 2024 •

edited

Loading

mapleFU commented Apr 23, 2024 •

edited

Loading

pitrou May 2, 2024

mapleFU May 8, 2024

pitrou May 2, 2024

mapleFU May 8, 2024

mapleFU May 8, 2024

rouault commented May 7, 2024

pitrou May 7, 2024

mapleFU May 8, 2024

mapleFU May 13, 2024

mapleFU commented May 7, 2024

mapleFU commented May 8, 2024

mapleFU May 8, 2024

mapleFU commented May 17, 2024

github-actions bot commented May 17, 2024

mapleFU commented May 20, 2024

conbench-apache-arrow bot commented May 21, 2024

		*values_read = this->ReadValues(values_to_read, values);
		ARROW_DCHECK_GE(values_to_read, *values_read);

GH-41321: [C++][Parquet] More strict Parquet level checking #41346

GH-41321: [C++][Parquet] More strict Parquet level checking #41346

Conversation

mapleFU commented Apr 23, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

mapleFU commented Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented May 7, 2024

mapleFU commented May 8, 2024

Choose a reason for hiding this comment

mapleFU commented May 17, 2024

github-actions bot commented May 17, 2024

mapleFU commented May 20, 2024

conbench-apache-arrow bot commented May 21, 2024

mapleFU commented Apr 23, 2024 •

edited

Loading

mapleFU commented Apr 23, 2024 •

edited

Loading