GH-39527: [C++][Parquet] Validate page sizes before truncating to int32 by emkornfield · Pull Request #39528 · apache/arrow

emkornfield · 2024-01-09T08:00:20Z

Be defensive instead of writing invalid data.

Rationale for this change

Users can provide this API pages that are large to validly write and we silently truncate lengths before writing.

What changes are included in this PR?

Add validations and throw an exception if sizes are too large (this was previously checked only if page indexes are being build).

Are these changes tested?

Unit tested

Are there any user-facing changes?

This might start raising exceptions instead of writing out invalid parquet files.

Closes #39527

This PR contains a "Critical Fix".

Closes: [C++][Parquet]: Page sizes are not always validated before truncating to int32 #39527

…o int32 Be defensive instead of writing invalid data.

emkornfield

fix includes

cpp/src/parquet/column_writer_test.cc

Co-authored-by: Gang Wu <[email protected]>

mapleFU

General LGTM!

mapleFU · 2024-01-10T03:52:43Z

cpp/src/parquet/column_writer_test.cc

+  DataPageV1 over_compressed_limit(buffer, /*num_values=*/100, Encoding::BIT_PACKED,
+                                   Encoding::BIT_PACKED, Encoding::BIT_PACKED,
+                                   /*uncompressed_size=*/100);
+  EXPECT_THROW(pager->WriteDataPage(over_compressed_limit), ParquetException);


Should we uses EXPECT_THAT to gurantee "overflows to INT32_MAX"?

mapleFU · 2024-01-10T03:53:23Z

cpp/src/parquet/column_writer.cc


    const uint8_t* output_data_buffer = compressed_data->data();
+    if (compressed_data->size() > std::numeric_limits<int32_t>::max()) {
+      throw ParquetException("Compressed page size overflows to INT32_MAX.");


should we note that the page type?

( Also can we print the page size? )

mapleFU · 2024-01-16T04:54:46Z

@emkornfield Could we move forward? I'll approve if comment fixed

emkornfield · 2024-01-16T05:01:15Z

Yes, will try to update this week.

emkornfield · 2024-01-22T21:41:47Z

@mapleFU apologies for the delay, I think I've addressed your feedback.

mapleFU

+1

mapleFU · 2024-01-24T04:51:58Z

cc @pitrou @wgtmac

cpp/src/parquet/column_writer.cc

pitrou · 2024-01-24T10:31:13Z

cpp/src/parquet/column_writer.cc


    const uint8_t* output_data_buffer = compressed_data->data();
+    if (compressed_data->size() > std::numeric_limits<int32_t>::max()) {
+      throw ParquetException(


Same here and below.

Co-authored-by: Antoine Pitrou <[email protected]>

pitrou

LGTM except for one last suggestion

pitrou · 2024-01-25T09:28:03Z

cpp/src/parquet/column_writer_test.cc

+              ThrowsMessage<ParquetException>(HasSubstr("overflows INT32_MAX")));
+  DictionaryPage dictionary_over_compressed_limit(buffer, /*num_values=*/100,
+                                                  Encoding::PLAIN);
+  EXPECT_THROW(pager->WriteDictionaryPage(dictionary_over_compressed_limit),


Sorry, didn't spot this, but perhaps also use EXPECT_THAT to check the exception message here?

I've updated this now

cpp/src/parquet/column_writer_test.cc

conbench-apache-arrow · 2024-01-27T09:33:19Z

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

There were no benchmark performance regressions. 🎉

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

…to int32 (apache#39528) Be defensive instead of writing invalid data. ### Rationale for this change Users can provide this API pages that are large to validly write and we silently truncate lengths before writing. ### What changes are included in this PR? Add validations and throw an exception if sizes are too large (this was previously checked only if page indexes are being build). ### Are these changes tested? Unit tested ### Are there any user-facing changes? This might start raising exceptions instead of writing out invalid parquet files. Closes apache#39527 **This PR contains a "Critical Fix".** * Closes: apache#39527 Lead-authored-by: emkornfield <[email protected]> Co-authored-by: Micah Kornfield <[email protected]> Co-authored-by: mwish <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Gang Wu <[email protected]> Signed-off-by: mwish <[email protected]>

…32 (#39528) Be defensive instead of writing invalid data. ### Rationale for this change Users can provide this API pages that are large to validly write and we silently truncate lengths before writing. ### What changes are included in this PR? Add validations and throw an exception if sizes are too large (this was previously checked only if page indexes are being build). ### Are these changes tested? Unit tested ### Are there any user-facing changes? This might start raising exceptions instead of writing out invalid parquet files. Closes #39527 **This PR contains a "Critical Fix".** * Closes: #39527 Lead-authored-by: emkornfield <[email protected]> Co-authored-by: Micah Kornfield <[email protected]> Co-authored-by: mwish <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Gang Wu <[email protected]> Signed-off-by: mwish <[email protected]>

apacheGH-39527: [C++][Parquet] Validate page sizes before truncated t…

fc89382

…o int32 Be defensive instead of writing invalid data.

emkornfield requested a review from wgtmac as a code owner January 9, 2024 08:00

github-actions bot added Component: Parquet Component: C++ awaiting committer review Awaiting committer review labels Jan 9, 2024

emkornfield commented Jan 9, 2024

View reviewed changes

fix include

69b5cbb

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Jan 9, 2024

wgtmac reviewed Jan 9, 2024

View reviewed changes

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

emkornfield and others added 2 commits January 9, 2024 12:54

Update cpp/src/parquet/column_writer_test.cc

2f9c906

Co-authored-by: Gang Wu <[email protected]>

move check earlier and add missing cast

e7fb451

mapleFU reviewed Jan 10, 2024

View reviewed changes

pitrou changed the title ~~GH-39527: [C++][Parquet] Validate page sizes before truncated to int32~~ GH-39527: [C++][Parquet] Validate page sizes before truncating to int32 Jan 16, 2024

address feedback

df8ca6c

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 22, 2024

emkornfield requested a review from mapleFU January 23, 2024 21:50

mapleFU approved these changes Jan 24, 2024

View reviewed changes

wgtmac approved these changes Jan 24, 2024

View reviewed changes

pitrou reviewed Jan 24, 2024

View reviewed changes

Update cpp/src/parquet/column_writer.cc

7d28e6a

Co-authored-by: Antoine Pitrou <[email protected]>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 25, 2024

update error message

3c57779

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 25, 2024

pitrou approved these changes Jan 25, 2024

View reviewed changes

mapleFU reviewed Jan 27, 2024

View reviewed changes

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

Update cpp/src/parquet/column_writer_test.cc

96f7047

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 27, 2024

mapleFU merged commit 5d7f661 into apache:main Jan 27, 2024

mapleFU removed the awaiting change review Awaiting change review label Jan 27, 2024

Conversation

emkornfield commented Jan 9, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mapleFU 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!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Jan 16, 2024

Uh oh!

emkornfield commented Jan 16, 2024

Uh oh!

emkornfield commented Jan 22, 2024

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Jan 24, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou 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!

Uh oh!

conbench-apache-arrow bot commented Jan 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emkornfield commented Jan 9, 2024 •

edited by github-actions bot

Loading