Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gszadovszky merged 4 commits into from
Sep 30, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d022451
PARQUET-2094: Handle negative values in page headers
gszadovszky Sep 22, 2021
c06a2c1
PARQUET-2094: Address comments
gszadovszky Sep 27, 2021
1346999
PARQUET-2094: Move exception to its own file
gszadovszky Sep 28, 2021
3d64e34
PARQUET-2094: Fix findings
gszadovszky Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions parquet-format-structures/src/main/java/org/apache/parquet/format/MetadataValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.parquet.format;

import java.io.IOException;
import java.util.function.Predicate;

/**
* Utility class to validate different types of Parquet metadata (e.g. footer, page headers etc.).
*/
public class MetadataValidator {

/**
* 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 {

@rdblue rdblue Sep 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@gszadovszky gszadovszky Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

private <T> InvalidParquetMetadataException(String metaName, T value) {
super("Metadata " + metaName + " is invalid: " + value);
}
}

static PageHeader validate(PageHeader pageHeader) throws InvalidParquetMetadataException {
validateValue(size -> size >= 0, pageHeader.getCompressed_page_size(), "pageHeader.compressed_page_size");
return pageHeader;
}

private static <T> void validateValue(Predicate<? super T> validator, T value, String metaName)

@rdblue rdblue Sep 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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));

@gszadovszky gszadovszky Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

throws InvalidParquetMetadataException {
if (!validator.test(value)) {
throw new InvalidParquetMetadataException(metaName, value);
}
}

private MetadataValidator() {
// Private constructor to prevent instantiation
}

}
3 changes: 2 additions & 1 deletion parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import static org.apache.parquet.format.FileMetaData._Fields.ROW_GROUPS;
import static org.apache.parquet.format.FileMetaData._Fields.SCHEMA;
import static org.apache.parquet.format.FileMetaData._Fields.VERSION;
import static org.apache.parquet.format.MetadataValidator.validate;
import static org.apache.parquet.format.event.Consumers.fieldConsumer;
import static org.apache.parquet.format.event.Consumers.listElementsOf;
import static org.apache.parquet.format.event.Consumers.listOf;
Expand Down Expand Up @@ -130,7 +131,7 @@ public static PageHeader readPageHeader(InputStream from) throws IOException {

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));

@rdblue rdblue Sep 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

}

public static void writeFileMetaData(org.apache.parquet.format.FileMetaData fileMetadata,
Expand Down
21 changes: 20 additions & 1 deletion parquet-format-structures/src/test/java/org/apache/parquet/format/TestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,17 @@
import static junit.framework.Assert.assertNull;
import static org.apache.parquet.format.Util.readFileMetaData;
import static org.apache.parquet.format.Util.writeFileMetaData;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;

import org.junit.Test;

import org.apache.parquet.format.MetadataValidator.InvalidParquetMetadataException;
import org.apache.parquet.format.Util.DefaultFileMetaDataConsumer;

@rdblue rdblue Sep 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: unnecessary change.


public class TestUtil {

@Test
Expand Down Expand Up @@ -77,6 +81,21 @@ public void testReadFileMetadata() throws Exception {
assertEquals(md, md6);
}

@Test
public void testInvalidPageHeader() throws IOException {
PageHeader ph = new PageHeader(PageType.DATA_PAGE, 100, -50);
ByteArrayOutputStream out = new ByteArrayOutputStream();
Util.writePageHeader(ph, out);

try {
Util.readPageHeader(in(out));
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"));

@rdblue rdblue Sep 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@gszadovszky gszadovszky Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

}
}

private ByteArrayInputStream in(ByteArrayOutputStream baos) {
return new ByteArrayInputStream(baos.toByteArray());
}
Expand Down