PARQUET-2094: Handle negative values in page headers by gszadovszky · Pull Request #933 · apache/parquet-java

gszadovszky · 2021-09-22T15:44:47Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-XXX
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

plygrnd · 2021-09-23T12:42:00Z

TravisCI runs contain multiple ParquetDecodingExceptions (example). Is this expected?

gszadovszky · 2021-09-23T14:23:57Z

TravisCI runs contain multiple ParquetDecodingExceptions (example). Is this expected?

@plygrnd, yes this is expected. The test class TestCorruptThriftRecords verifies if the correct exceptions are thrown. It uses MR to execute the tests and it seems MR logs the exceptions before they are thrown to the caller.

plygrnd · 2021-09-23T14:32:42Z

Okay, LGTM then.

shangxinli · 2021-09-23T15:20:46Z

LGTM

rdblue · 2021-09-23T21:03:24Z

parquet-format-structures/src/main/java/org/apache/parquet/format/MetadataValidator.java

+   * A specific IOException thrown when invalid values are found in the Parquet file metadata (including the footer,
+   * page header etc.).
+   */
+  public static class InvalidParquetMetadataException extends IOException {


I don't think that this is an IOException. What is the value of making this a checked exception? Why not just make this a RuntimeException? Or use some existing one like IllegalStateException or ParquetDecodingException?

The module parquet-format-structures is the one all the others are depended on. Parquet exceptions are implemented in another module so I cannot use them here. Since we already throw IOExceptions I've felt extending it would be a good idea. But you might be right. I am happy to extend RuntimeException instead of IOException.

rdblue · 2021-09-23T21:05:36Z

parquet-format-structures/src/main/java/org/apache/parquet/format/MetadataValidator.java

+    return pageHeader;
+  }
+
+  private static <T> void validateValue(Predicate<? super T> validator, T value, String metaName)


Why accept a predicate? Most check methods like this use a boolean. I would expect it to work like a Precondition:

if (!isValid) { throw new ParquetDecodingException(...); } int size = pageHeader.getCompressed_page_size() validateValue(size >= 0, String.format("Compressed page size must be positive, but was: %s", size));

I am not sure why I've implemented this way. I'm fine rewriting to use a simple boolean.

rdblue · 2021-09-23T21:06:21Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java

  public static PageHeader readPageHeader(InputStream from, 
      BlockCipher.Decryptor decryptor, byte[] AAD) throws IOException {
-    return read(from, new PageHeader(), decryptor, AAD);
+    return validate(read(from, new PageHeader(), decryptor, AAD));


I think it would be more clear if you called MetadataValidator.validate rather than just validate.

rdblue · 2021-09-23T21:07:17Z

parquet-format-structures/src/test/java/org/apache/parquet/format/TestUtil.java

+      fail("Expected exception but did not thrown");
+    } catch (InvalidParquetMetadataException e) {
+      assertTrue("Exception message does not contain the expected parts",
+          e.getMessage().contains("pageHeader.compressed_page_size"));


Isn't there an assertion helper so you don't need to catch the exception? Something like assertThrows in the codebase?

Yes, there is something already implemented but in another module and I cannot use it here.

rdblue · 2021-09-27T19:12:00Z

parquet-format-structures/src/main/java/org/apache/parquet/format/MetadataValidator.java

+   * A specific RuntimeException thrown when invalid values are found in the Parquet file metadata (including the
+   * footer, page header etc.).
+   */
+  public static class InvalidParquetMetadataException extends RuntimeException {


Minor: I'd prefer it if the exception weren't an inner class since that makes it harder to reference. But this isn't a blocker.

It is a fair point anyway. I'll move it before merging. Thanks a lot for the review.

rdblue · 2021-09-28T16:49:12Z

...rmat-structures/src/main/java/org/apache/parquet/format/InvalidParquetMetadataException.java

+ * page header etc.).
+ */
+public class InvalidParquetMetadataException extends RuntimeException {
+  <T> InvalidParquetMetadataException(String message) {


What is the type parameter for?

rdblue · 2021-09-28T16:49:50Z

parquet-format-structures/src/test/java/org/apache/parquet/format/TestUtil.java

+import org.apache.parquet.format.Util.DefaultFileMetaDataConsumer;
 import org.junit.Test;

-import org.apache.parquet.format.Util.DefaultFileMetaDataConsumer;


Nit: unnecessary change.

(cherry picked from commit 1695d92)

rdblue · 2021-10-03T21:22:10Z

@gszadovszky, thanks for getting this done.

(cherry picked from commit 1695d92)

PARQUET-2094: Handle negative values in page headers

d022451

shangxinli approved these changes Sep 23, 2021

View reviewed changes

rdblue reviewed Sep 23, 2021

View reviewed changes

PARQUET-2094: Address comments

c06a2c1

rdblue reviewed Sep 27, 2021

View reviewed changes

rdblue approved these changes Sep 27, 2021

View reviewed changes

PARQUET-2094: Move exception to its own file

1346999

rdblue reviewed Sep 28, 2021

View reviewed changes

PARQUET-2094: Fix findings

3d64e34

gszadovszky merged commit 1695d92 into apache:master Sep 30, 2021

gszadovszky added a commit that referenced this pull request Sep 30, 2021

PARQUET-2094: Handle negative values in page headers (#933)

af713b5

(cherry picked from commit 1695d92)

gszadovszky added a commit that referenced this pull request Sep 30, 2021

PARQUET-2094: Handle negative values in page headers (#933)

afb64b8

(cherry picked from commit 1695d92)

sunchao pushed a commit to sunchao/parquet-mr that referenced this pull request Mar 3, 2022

PARQUET-2094: Handle negative values in page headers (apache#933)

69e0622

(cherry picked from commit 1695d92)

Conversation

gszadovszky commented Sep 22, 2021

Jira

Tests

Commits

Documentation

Uh oh!

plygrnd commented Sep 23, 2021

Uh oh!

gszadovszky commented Sep 23, 2021

Uh oh!

plygrnd commented Sep 23, 2021

Uh oh!

shangxinli commented Sep 23, 2021

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!

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!

rdblue commented Oct 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants