[
https://issues.apache.org/jira/browse/HADOOP-19930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108592#comment-18108592
]
ASF GitHub Bot commented on HADOOP-19930:
-----------------------------------------
ajfabbri commented on code in PR #8640:
URL: https://github.com/apache/hadoop/pull/8640#discussion_r3866730256
##########
.github/workflows/update_build_status.yml:
##########
@@ -29,6 +29,7 @@ jobs:
update:
name: "Update Build Status"
runs-on: ubuntu-slim
+ if: github.repository == 'apache/hadoop'
Review Comment:
i believe this was already included in #8702
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/WritableUtils.java:
##########
@@ -495,4 +495,5 @@ public static String readStringSafely(DataInput in,
in.readFully(bytes, 0, length);
return Text.decode(bytes);
}
+
Review Comment:
nit: superfluous whitespace change
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestWritableClassValidation.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * 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;
+
+import java.io.IOException;
+import java.util.EnumSet;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+import org.apache.hadoop.util.ReflectionUtils;
+
+import static org.apache.hadoop.io.ObjectWritable.E_MAX_DEPTH;
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.fail;
+
+/**
+ * Tests that the polymorphic Writable classes reject a class named on the wire
+ * that does not implement the required type, and do not load that class.
+ */
+public class TestWritableClassValidation extends AbstractHadoopTestBase {
+
+ /** Name of {@link Sentinel}, referenced as a string so it is never loaded.
*/
+ private static final String SENTINEL =
+ TestWritableClassValidation.class.getName() + "$Sentinel";
+
+ @BeforeEach
+ public void resetSentinel() {
+ LoadFlag.setLoaded(false);
+ }
+
+ @Test
+ public void testLoadClassRejectsWrongType() throws Exception {
+ Configuration conf = new Configuration();
+ intercept(ClassCastException.class, () -> {
+ ReflectionUtils.loadUninitedClass(conf, SENTINEL, Writable.class);
+ });
+ assertSentinelNotLoaded();
+ }
+
+ private static void assertSentinelNotLoaded() {
+ assertThat(LoadFlag.isLoaded()).
+ describedAs("sentinel must not be loaded")
+ .isFalse();
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testObjectWritableRejectsNonWritable() throws Exception {
+ DataOutputBuffer out = new DataOutputBuffer();
+ // A benign declaredClass routes readObject down the Writable branch; the
+ // instanceClass then names the non-Writable sentinel, which is the value
+ // checked before instantiation.
+ UTF8.writeString(out, Text.class.getName());
+ UTF8.writeString(out, SENTINEL);
+ DataInputBuffer in = new DataInputBuffer();
+ in.reset(out.getData(), out.getLength());
+ assertWrongKind(intercept(RuntimeException.class,
+ () -> ObjectWritable.readObject(in, null)));
+ assertSentinelNotLoaded();
+ }
+
+ /** Assert a ClassCastException is somewhere in the cause chain. */
+ private static void assertWrongKind(Throwable t) {
+ for (Throwable c = t; c != null; c = c.getCause()) {
+ if (c instanceof ClassCastException) {
+ return;
+ }
+ }
+ fail("expected ClassCastException in cause chain of " + t);
+ }
+
+ @Test
+ public void testMapWritableRejectsNonWritable() throws Exception {
+ DataOutputBuffer out = new DataOutputBuffer();
+ out.writeByte(1); // one "new" class in the table
+ out.writeByte(1); // its id
+ out.writeUTF(SENTINEL); // its name
+ DataInputBuffer in = new DataInputBuffer();
+ in.reset(out.getData(), out.getLength());
+ MapWritable map = new MapWritable();
+ intercept(ClassCastException.class, () ->
+ map.readFields(in));
+ assertSentinelNotLoaded();
+ }
+
+ @Test
+ public void testEnumSetWritableRejectsNonEnum() throws Exception {
+ DataOutputBuffer out = new DataOutputBuffer();
+ out.writeInt(0); // empty set
+ WritableUtils.writeString(out, SENTINEL); // element type
+ DataInputBuffer in = new DataInputBuffer();
+ in.reset(out.getData(), out.getLength());
+ EnumSetWritable<?> set = new EnumSetWritable<>();
+ assertWrongKind(intercept(RuntimeException.class, () ->
set.readFields(in)));
+ assertSentinelNotLoaded();
+ }
+
+ @Test
+ public void testNegativeArrayLengthRejected() throws Exception {
+ DataOutputBuffer out = new DataOutputBuffer();
+ UTF8.writeString(out, Text[].class.getName());
+ out.writeInt(-1);
+ DataInputBuffer in = new DataInputBuffer();
+ in.reset(out.getData(), out.getLength());
+ intercept(IOException.class, () -> ObjectWritable.readObject(in, null));
+ }
+
+ @Test
+ public void testHugeArrayLengthDoesNotPreallocate() throws Exception {
+ DataOutputBuffer out = new DataOutputBuffer();
+ UTF8.writeString(out, Text[].class.getName());
+ out.writeInt(Integer.MAX_VALUE); // no element data follows
+ DataInputBuffer in = new DataInputBuffer();
+ in.reset(out.getData(), out.getLength());
+ intercept(IOException.class, () -> ObjectWritable.readObject(in, null));
+ }
+
+ @Test
+ public void testDeepNestingRejected() throws Exception {
+ DataOutputBuffer out = new DataOutputBuffer();
+ String arrayClass = Object[].class.getName();
+ for (int i = 0; i < 110; i++) { // each level: an Object[] of length 1
Review Comment:
Not a blocking comment, but a constant for 100 depth limit and using it here
(X + 10) would be clearer to future generations.
> ObjectWritable to take interface/superclass and check before class load
> -----------------------------------------------------------------------
>
> Key: HADOOP-19930
> URL: https://issues.apache.org/jira/browse/HADOOP-19930
> Project: Hadoop Common
> Issue Type: Improvement
> Components: ipc
> Affects Versions: 3.5.0, 3.4.3, 3.5.1
> Reporter: Steve Loughran
> Assignee: Steve Loughran
> Priority: Minor
> Labels: pull-request-available
>
> ObjectWritable.loadClass is used to load classes from writables, after which
> the loaded class is cast to the target interface and then instantiated.
> Validating that the loaded class is of the required type prior to
> instantiation allows for faster failing, as there's no load.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
