[
https://issues.apache.org/jira/browse/HADOOP-19936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095295#comment-18095295
]
ASF GitHub Bot commented on HADOOP-19936:
-----------------------------------------
Copilot commented on code in PR #8594:
URL: https://github.com/apache/hadoop/pull/8594#discussion_r3559230233
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/file/tfile/Utils.java:
##########
@@ -274,8 +275,36 @@ public static void writeString(DataOutput out, String s)
throws IOException {
* @throws IOException raised on errors performing I/O.
*/
public static String readString(DataInput in) throws IOException {
+ return readString(in, -1);
+ }
+
+ /**
+ * Read a String as a VInt n, followed by n Bytes in Text format, rejecting
+ * lengths that are -2 or less. or larger than the supplied bound before any
+ * buffer is allocated.
Review Comment:
Javadoc has a sentence break/typo ("-2 or less. or larger") which makes the
description harder to read. Consider rephrasing to a single sentence with
proper punctuation.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/file/tfile/TestTFileLoading.java:
##########
@@ -0,0 +1,190 @@
+/*
+ * 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.hadoop.io.file.tfile;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.DataInputStream;
+import java.io.DataOutputStream;
+import java.io.EOFException;
+import java.io.IOException;
+
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.io.file.tfile.TFile.Reader;
+import org.apache.hadoop.io.file.tfile.TFile.Writer;
+import org.apache.hadoop.test.GenericTestUtils;
+
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Test TFile loading resilience.
+ */
+public class TestTFileLoading {
+
+ private static final String ROOT =
+ GenericTestUtils.getTestDir().getAbsolutePath();
+
+ private static final int BLOCK_SIZE = 512;
+
+ private static final String COMPRESSION =
Compression.Algorithm.NONE.getName();
+
+ private static final String JCLASS_COMPARATOR =
+ TFile.COMPARATOR_JCLASS + LongWritable.Comparator.class.getName();
+
+ private FileSystem fs;
+
+ private Path path;
+
+ @BeforeEach
+ public void setUp() throws IOException {
+ Configuration conf = new Configuration();
+ path = new Path(ROOT, "TestTFileLoading");
+ fs = FileSystem.getLocal(conf);
+ }
+
+ @AfterEach
+ public void tearDown() throws IOException {
+ fs.delete(path, true);
+ }
+
+ private Configuration enableJclass() {
+ Configuration conf = new Configuration();
+ conf.setBoolean(TFile.TFILE_COMPARATOR_JCLASS_ENABLED, true);
+ return conf;
+ }
+
+ @Test
+ public void testJClassComparatorRejectedByDefault() throws Exception {
+ try (FSDataOutputStream out = fs.create(path)) {
+ intercept(IllegalArgumentException.class, () ->
+ new Writer(out, BLOCK_SIZE, COMPRESSION, JCLASS_COMPARATOR, new
Configuration()));
+ }
+ }
+
+ @Test
+ public void testJClassComparatorMustBeRawComparator() throws Exception {
+ Configuration conf = enableJclass();
+ String notAComparator =
+ TFile.COMPARATOR_JCLASS + Chunk.class.getName();
+ try (FSDataOutputStream out = fs.create(path)) {
+ intercept(IllegalArgumentException.class, () ->
+ new Writer(out, BLOCK_SIZE, COMPRESSION, notAComparator, conf));
+ }
+ }
+
+ /**
+ * A file that declares a jclass comparator (written with the feature
+ * enabled) is not honored by a reader that has not enabled it; the named
+ * class is only resolved and constructed when the feature is turned on.
+ */
+ @Test
+ public void testReaderIgnoresJClassWhenDisabled() throws Exception {
Review Comment:
The test name says the reader "ignores" `jclass:` comparators when disabled,
but the test asserts the reader rejects the file. Renaming the test improves
clarity and matches the behavior being verified.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/file/tfile/TFile.java:
##########
@@ -2048,27 +2063,32 @@ static final class TFileMeta {
private final BytesComparator comparator;
// ctor for writes
- public TFileMeta(String comparator) {
+ TFileMeta(String comparator, Configuration conf) {
// set fileVersion to API version when we create it.
version = TFile.API_VERSION;
recordCount = 0;
strComparator = (comparator == null) ? "" : comparator;
- this.comparator = makeComparator(strComparator);
+ this.comparator = makeComparator(strComparator, conf);
}
// ctor for reads
- public TFileMeta(DataInput in) throws IOException {
+ TFileMeta(DataInput in, Configuration conf) throws IOException {
version = new Version(in);
if (!version.compatibleWith(TFile.API_VERSION)) {
throw new RuntimeException("Incompatible TFile fileVersion.");
}
recordCount = Utils.readVLong(in);
- strComparator = Utils.readString(in);
- comparator = makeComparator(strComparator);
+ strComparator = Utils.readString(in, MAX_COMPARATOR_LENGTH);
+ comparator = makeComparator(strComparator, conf);
}
- @SuppressWarnings("unchecked")
static BytesComparator makeComparator(String comparator) {
+ return makeComparator(comparator, new Configuration());
+ }
Review Comment:
`TFileMeta.makeComparator(String)` creates a full default `Configuration`,
which can load default resources and is relatively expensive for a pure parsing
helper. Using `new Configuration(false)` avoids that overhead while keeping
behavior the same (and callers that need settings should use the `(String,
Configuration)` overload).
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/file/tfile/TFile.java:
##########
@@ -166,6 +166,21 @@ static int getFSOutputBufferSize(Configuration conf) {
/** comparator prefix: java class */
public static final String COMPARATOR_JCLASS = "jclass:";
+ /**
+ * {@value}.
+ */
+ public static final String TFILE_COMPARATOR_JCLASS_ENABLED =
+ "tfile.comparator.jclass.enabled";
+ public static final boolean TFILE_COMPARATOR_JCLASS_ENABLED_DEFAULT = false;
+
Review Comment:
With `TFILE_COMPARATOR_JCLASS_ENABLED` now gating `jclass:` comparator
resolution, the public `TFile.makeComparator(String)` helper has no way to
accept a `Configuration` with that flag enabled. Adding an overload which takes
a `Configuration` lets callers opt in programmatically (and keeps the default
behavior locked down).
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/file/tfile/TestTFileComparators.java:
##########
@@ -73,45 +73,28 @@ public void tearDown() throws IOException {
// bad comparator format
@Test
- public void testFailureBadComparatorNames() throws IOException {
- try {
- writer = new Writer(out, BLOCK_SIZE, compression, "badcmp", conf);
- fail("Failed to catch unsupported comparator names");
- }
- catch (Exception e) {
- // noop, expecting exceptions
- e.printStackTrace();
- }
+ public void testFailureBadComparatorNames() throws Exception {
+ intercept(IllegalArgumentException.class, "Unsupported comparator", () ->
+ new Writer(out, BLOCK_SIZE, compression, "badcmp", conf));
}
Review Comment:
`setUp()` creates and leaves `out` open, and these tests intentionally
trigger exceptions during `Writer` construction. Without closing `out`, this
can leak file descriptors and may cause intermittent failures on some
platforms. Close `out` in `tearDown()` (you already have `closeOutput()` for
this).
> TFile parsing is too brittle
> ----------------------------
>
> Key: HADOOP-19936
> URL: https://issues.apache.org/jira/browse/HADOOP-19936
> Project: Hadoop Common
> Issue Type: Improvement
> Components: io
> Affects Versions: 3.5.0, 3.4.3
> Reporter: Steve Loughran
> Assignee: Steve Loughran
> Priority: Minor
> Labels: pull-request-available
>
> TFile parser doesn't validate its inputs before allocating memory or
> attempting to instantiate a class. This is old code, but still used in places.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
