From bdf22377ff0a87a289f7e641c01ee48f0f16e4a2 Mon Sep 17 00:00:00 2001 From: okumin Date: Wed, 26 Aug 2026 22:37:28 +0900 Subject: [PATCH 1/8] Add validations to PartitionExpressionForMetastore Generated-by: Claude --- .../hive/ql/exec/SerializationUtilities.java | 122 ++++++++++++++++-- .../ppr/PartitionExpressionForMetastore.java | 56 +++++++- .../ql/exec/TestSerializationUtilities.java | 68 ++++++++++ .../TestPartitionExpressionForMetastore.java | 85 ++++++++++++ 4 files changed, 315 insertions(+), 16 deletions(-) create mode 100644 ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java diff --git a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java index b61f4d484439..2cf8c67c2997 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java @@ -32,12 +32,14 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Properties; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -58,6 +60,7 @@ import org.apache.hadoop.hive.ql.plan.MapredWork; import org.apache.hadoop.hive.ql.plan.PartitionDesc; import org.apache.hadoop.hive.ql.session.SessionState; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDF; import org.apache.hadoop.hive.serde2.Serializer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -126,8 +129,10 @@ private static class KryoWithHooks extends Kryo implements Configurable { private Hook globalHook; // this should be set on-the-fly after borrowing this instance and needs to be reset on release private Configuration configuration; - // default false, should be reset on release - private boolean isExprNodeFirst = false; + // when non-null, the stream being deserialized is untrusted: the first class read must be + // compatible with this type and every class read must pass + // isAllowedForUntrustedDeserialization(); default null (trusted), reset on release + private Class untrustedRootType = null; // total classes we have met during (de)serialization, should be reset on release private long classCounter = 0; @@ -237,13 +242,22 @@ public Configuration getConf() { @Override public com.esotericsoftware.kryo.kryo5.Registration getRegistration(Class type) { - // If PartitionExpressionForMetastore performs deserialization at remote HMS, - // the first class encountered during deserialization must be an ExprNodeDesc, - // throw exception to avoid potential security problem if it is not. - if (isExprNodeFirst && classCounter == 0) { - if (!ExprNodeDesc.class.isAssignableFrom(type)) { + // If this instance deserializes a payload that a remote client controls (e.g. + // PartitionExpressionForMetastore at a remote HMS) or that a client can persist (e.g. a + // table property copied into the job conf), the first class encountered during + // deserialization must be compatible with the expected root type, and every class in the + // stream must pass the allowlist check. Kryo is otherwise willing to instantiate any + // classpath class named by the payload (registrationRequired=false plus + // StdInstantiatorStrategy), which turns these payloads into a + // deserialization-of-untrusted-data primitive. + if (untrustedRootType != null) { + if (classCounter == 0 && !untrustedRootType.isAssignableFrom(type)) { + throw new UnsupportedOperationException("The object to be deserialized must be a " + + untrustedRootType.getName() + ", but encountered: " + type); + } + if (!isAllowedForUntrustedDeserialization(type)) { throw new UnsupportedOperationException( - "The object to be deserialized must be an ExprNodeDesc, but encountered: " + type); + "Deserialization of " + type + " is not allowed from an untrusted payload"); } } classCounter++; @@ -251,17 +265,81 @@ public com.esotericsoftware.kryo.kryo5.Registration getRegistration(Class type) } public void setExprNodeFirst(boolean isPartFilter) { - this.isExprNodeFirst = isPartFilter; + setUntrustedRootType(isPartFilter ? ExprNodeDesc.class : null); + } + + void setUntrustedRootType(Class rootType) { + this.untrustedRootType = rootType; + this.classCounter = 0; } // reset the fields on release public void restore() { setConf(null); - isExprNodeFirst = false; + untrustedRootType = null; classCounter = 0; } } + /** + * Package prefixes that classes read from an untrusted Kryo payload may come from. These cover + * everything a legitimate serialized expression ({@link ExprNodeDesc} graph) or search argument + * (SearchArgumentImpl graph) contains: expression descriptors and plan literals, builtin and + * installed UDFs, type infos and object inspectors, Hive/Hadoop value types, and plain JDK + * value/collection classes. Known gadget carriers (commons-collections, beanutils, + * xalan/TemplatesImpl, ...) all live outside these prefixes. + */ + private static final String[] UNTRUSTED_ALLOWED_PACKAGE_PREFIXES = new String[] { + "java.lang.", + "java.util.", + "java.sql.", + "java.time.", + "java.math.", + "org.apache.hadoop.hive.ql.plan.", + "org.apache.hadoop.hive.ql.udf.", + "org.apache.hadoop.hive.ql.io.sarg.", + "org.apache.hadoop.hive.serde2.", + "org.apache.hadoop.hive.common.type.", + "org.apache.hadoop.io." + }; + + /** + * Classes that are never acceptable in an untrusted payload even though they pass the package + * allowlist: reflect()/reflect2() invoke arbitrary methods on arbitrary classes, so a + * pre-instantiated instance arriving in a client-supplied expression is an + * arbitrary-code-execution primitive for whoever evaluates the expression. + */ + private static final Set UNTRUSTED_DENIED_CLASS_NAMES = new HashSet<>(Arrays.asList( + "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect", + "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2")); + + @VisibleForTesting + static boolean isAllowedForUntrustedDeserialization(Class type) { + Class component = type; + while (component.isArray()) { + component = component.getComponentType(); + } + if (component.isPrimitive()) { + return true; + } + String name = component.getName(); + if (UNTRUSTED_DENIED_CLASS_NAMES.contains(name)) { + return false; + } + // Custom (temporary/permanent) UDFs live in user packages. The classes themselves were + // installed by an administrator, so allowing kryo to instantiate them is no worse than any + // query invoking them. + if (GenericUDF.class.isAssignableFrom(component) || UDF.class.isAssignableFrom(component)) { + return true; + } + for (String prefix : UNTRUSTED_ALLOWED_PACKAGE_PREFIXES) { + if (name.startsWith(prefix)) { + return true; + } + } + return false; + } + private static final Object FAKE_REFERENCE = new Object(); // Bounded queue could be specified here but that will lead to blocking. @@ -883,7 +961,29 @@ public static String serializeExpression(ExprNodeGenericFuncDesc expr) { public static ExprNodeGenericFuncDesc deserializeExpression(String s) { byte[] bytes = Base64.decodeBase64(s.getBytes(StandardCharsets.UTF_8)); - return deserializeObjectFromKryo(bytes, ExprNodeGenericFuncDesc.class); + // Serialized expressions travel through configuration values (e.g. + // hive.io.filter.expr.serialized) that clients can shadow via table properties or SET, so + // they must always be deserialized with the untrusted-payload restrictions. + return deserializeUntrustedObjectFromKryo(bytes, ExprNodeGenericFuncDesc.class); + } + + /** + * Deserializes bytes that a client may control (a remote-supplied expression, a value read back + * from a configuration key that table properties can shadow, ...). In addition to pinning the + * root object to {@code clazz}, every class named in the stream is validated against a fixed + * allowlist, so the payload cannot make Kryo instantiate arbitrary classpath classes. + * @param bytes Bytes containing the object. + * @param clazz The expected class of the root object. + * @return The deserialized object. + */ + public static T deserializeUntrustedObjectFromKryo(byte[] bytes, Class clazz) { + KryoWithHooks kryo = (KryoWithHooks) borrowKryo(); + kryo.setUntrustedRootType(clazz); + try (Input inp = new Input(new ByteArrayInputStream(bytes))) { + return kryo.readObject(inp, clazz); + } finally { + releaseKryo(kryo); + } } public static byte[] serializeObjectToKryo(Serializable object) { diff --git a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java index 522c9896684f..6e21348fca4b 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java @@ -30,15 +30,19 @@ import org.apache.hadoop.hive.metastore.PartitionExpressionProxy; import org.apache.hadoop.hive.metastore.api.MetaException; import org.apache.hadoop.hive.ql.exec.SerializationUtilities; +import org.apache.hadoop.hive.ql.exec.UDF; import org.apache.hadoop.hive.ql.io.orc.OrcFileFormatProxy; import org.apache.hadoop.hive.ql.io.orc.OrcInputFormat; -import org.apache.hadoop.hive.ql.io.sarg.ConvertAstToSearchArg; import org.apache.hadoop.hive.ql.io.sarg.SearchArgument; +import org.apache.hadoop.hive.ql.io.sarg.SearchArgumentImpl; import org.apache.hadoop.hive.ql.metadata.HiveException; import org.apache.hadoop.hive.ql.parse.SemanticException; import org.apache.hadoop.hive.ql.plan.ExprNodeDesc; import org.apache.hadoop.hive.ql.plan.ExprNodeDescUtils; import org.apache.hadoop.hive.ql.plan.ExprNodeGenericFuncDesc; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDF; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFBridge; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFMacro; import org.apache.hadoop.hive.serde2.typeinfo.PrimitiveTypeInfo; import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory; import org.slf4j.Logger; @@ -111,21 +115,61 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes) throws MetaException { try { expr = SerializationUtilities.deserializeObjectWithTypeInformation(exprBytes, true); } catch (Exception ex) { - LOG.error("Failed to deserialize the expression, fall back to deserializeObjectFromKryo", ex); + LOG.error("Failed to deserialize the expression, fall back to deserializeUntrustedObjectFromKryo", ex); try { - expr = SerializationUtilities.deserializeObjectFromKryo(exprBytes, ExprNodeGenericFuncDesc.class); + // The fallback must use the same untrusted-payload restrictions as the primary path: + // these bytes come straight from a Thrift client. + expr = SerializationUtilities.deserializeUntrustedObjectFromKryo(exprBytes, ExprNodeGenericFuncDesc.class); } catch (Exception e) { LOG.error("Failed to deserialize the expression", e); throw new MetaException("SerializationUtilities#deserializeObjectWithTypeInformation: " + ex.getMessage() + - ", SerializationUtilities#deserializeObjectFromKryo: " + e.getMessage()); + ", SerializationUtilities#deserializeUntrustedObjectFromKryo: " + e.getMessage()); } } if (expr == null) { throw new MetaException("Failed to deserialize expression - ExprNodeDesc not present"); } + validateDeserializedExpr(expr); return expr; } + /** + * Rejects client-supplied expression graphs that would execute arbitrary code when the + * metastore stringifies or evaluates them. The Kryo-level class allowlist already blocks + * reflect()/reflect2(); a {@link GenericUDFBridge} instance is legitimate (it wraps builtin + * old-style UDFs like year()), but it instantiates whatever class name its + * {@code udfClassName} field carries, so that name must resolve to a real {@link UDF}. + */ + private void validateDeserializedExpr(ExprNodeDesc expr) throws MetaException { + if (expr instanceof ExprNodeGenericFuncDesc) { + GenericUDF genericUDF = ((ExprNodeGenericFuncDesc) expr).getGenericUDF(); + if (genericUDF instanceof GenericUDFBridge) { + String udfClassName = ((GenericUDFBridge) genericUDF).getUdfClassName(); + Class udfClass; + try { + udfClass = Class.forName(udfClassName, false, Thread.currentThread().getContextClassLoader()); + } catch (ClassNotFoundException | LinkageError e) { + throw new MetaException("Unknown UDF class in partition filter expression: " + udfClassName); + } + if (!UDF.class.isAssignableFrom(udfClass)) { + throw new MetaException("Class in partition filter expression is not a UDF: " + udfClassName); + } + } + if (genericUDF instanceof GenericUDFMacro) { + // a macro body is an expression graph of its own + ExprNodeDesc body = ((GenericUDFMacro) genericUDF).getBody(); + if (body != null) { + validateDeserializedExpr(body); + } + } + } + if (expr.getChildren() != null) { + for (ExprNodeDesc child : expr.getChildren()) { + validateDeserializedExpr(child); + } + } + } + @Override public FileFormatProxy getFileFormatProxy(FileMetadataExprType type) { switch (type) { @@ -150,6 +194,8 @@ public FileMetadataExprType getMetadataType(String inputFormat) { @Override public SearchArgument createSarg(byte[] expr) { - return ConvertAstToSearchArg.create(expr); + // These bytes also come straight from a Thrift client (get_file_metadata_by_expr), so they + // get the same untrusted-payload restrictions as the partition filter expressions above. + return SerializationUtilities.deserializeUntrustedObjectFromKryo(expr, SearchArgumentImpl.class); } } diff --git a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java index 0c003d5e46de..6b72db1bcd98 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java +++ b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java @@ -46,7 +46,9 @@ import org.apache.hadoop.hive.ql.plan.PartitionDesc; import org.apache.hadoop.hive.ql.plan.TableDesc; import org.apache.hadoop.hive.ql.plan.VectorPartitionDesc; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPEqual; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPNull; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect; import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory; import org.junit.Assert; import org.junit.Test; @@ -246,4 +248,70 @@ private static MapWork mockMapWorkWithSomePartitionDescProperties() throws Excep return mapWork; } + + @Test + public void testUntrustedDeserializationAcceptsLegitimateExpression() throws Exception { + ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( + TypeInfoFactory.stringTypeInfo, "value")); + + byte[] typed = SerializationUtilities.serializeObjectWithTypeInformation(expr); + Object deserialized = SerializationUtilities.deserializeObjectWithTypeInformation(typed, true); + Assert.assertTrue(deserialized instanceof ExprNodeGenericFuncDesc); + + String base64 = SerializationUtilities.serializeExpression(expr); + Assert.assertNotNull(SerializationUtilities.deserializeExpression(base64)); + } + + @Test(expected = UnsupportedOperationException.class) + public void testUntrustedDeserializationRejectsNonExprRoot() throws Exception { + byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation( + new LinkedHashMap()); + SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); + } + + @Test(expected = UnsupportedOperationException.class) + public void testUntrustedDeserializationRejectsSmuggledClass() throws Exception { + // a class outside the allowlist carried in a "constant" stands in for a gadget object + ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( + TypeInfoFactory.stringTypeInfo, new java.io.File("/tmp/x"))); + byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(expr); + SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); + } + + @Test(expected = UnsupportedOperationException.class) + public void testDeserializeExpressionRejectsSmuggledClass() throws Exception { + ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( + TypeInfoFactory.stringTypeInfo, new java.io.File("/tmp/x"))); + SerializationUtilities.deserializeExpression(SerializationUtilities.serializeExpression(expr)); + } + + @Test(expected = UnsupportedOperationException.class) + public void testUntrustedDeserializationRejectsReflectUdf() throws Exception { + List children = new ArrayList<>(); + children.add(new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); + ExprNodeGenericFuncDesc expr = new ExprNodeGenericFuncDesc(TypeInfoFactory.stringTypeInfo, + new GenericUDFReflect(), children); + byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(expr); + SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); + } + + @Test + public void testUntrustedDeserializationAllowlist() { + Assert.assertTrue( + SerializationUtilities.isAllowedForUntrustedDeserialization(ExprNodeGenericFuncDesc.class)); + Assert.assertTrue(SerializationUtilities.isAllowedForUntrustedDeserialization(GenericUDFOPNull.class)); + Assert.assertTrue(SerializationUtilities.isAllowedForUntrustedDeserialization(ArrayList.class)); + Assert.assertTrue(SerializationUtilities.isAllowedForUntrustedDeserialization(byte[].class)); + Assert.assertFalse(SerializationUtilities.isAllowedForUntrustedDeserialization(java.io.File.class)); + Assert.assertFalse(SerializationUtilities.isAllowedForUntrustedDeserialization(Path.class)); + Assert.assertFalse(SerializationUtilities.isAllowedForUntrustedDeserialization(GenericUDFReflect.class)); + } + + private static ExprNodeGenericFuncDesc buildColumnEqualsConstant(ExprNodeConstantDesc constant) { + List children = new ArrayList<>(); + children.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "col1", "tab", false)); + children.add(constant); + return new ExprNodeGenericFuncDesc(TypeInfoFactory.booleanTypeInfo, + new GenericUDFOPEqual(), children); + } } diff --git a/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java new file mode 100644 index 000000000000..21358bb08170 --- /dev/null +++ b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java @@ -0,0 +1,85 @@ +/* + * 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.hive.ql.optimizer.ppr; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.hadoop.hive.metastore.api.MetaException; +import org.apache.hadoop.hive.ql.exec.SerializationUtilities; +import org.apache.hadoop.hive.ql.plan.ExprNodeColumnDesc; +import org.apache.hadoop.hive.ql.plan.ExprNodeConstantDesc; +import org.apache.hadoop.hive.ql.plan.ExprNodeDesc; +import org.apache.hadoop.hive.ql.plan.ExprNodeGenericFuncDesc; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDF; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFBridge; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPEqual; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect; +import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory; +import org.junit.Assert; +import org.junit.Test; + +/** + * Verifies that the metastore-side expression deserialization only accepts benign expression + * graphs: the expression bytes arrive straight from Thrift clients, so classes outside the + * allowlist, reflect()/reflect2(), and GenericUDFBridge instances pointing at non-UDF classes + * must all be rejected before anything stringifies or evaluates the expression. + */ +public class TestPartitionExpressionForMetastore { + + @Test + public void testComparisonExpressionIsAccepted() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFOPEqual(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "2026-08-11")); + String filter = new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + Assert.assertNotNull(filter); + } + + @Test(expected = MetaException.class) + public void testReflectUdfIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFReflect(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + @Test(expected = MetaException.class) + public void testBridgeToNonUdfClassIsRejected() throws Exception { + GenericUDFBridge bridge = new GenericUDFBridge("evil", false, "java.lang.ProcessBuilder"); + ExprNodeGenericFuncDesc expr = buildExpression(bridge, + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "x")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + @Test(expected = MetaException.class) + public void testSmuggledClassIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFOPEqual(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, new java.io.File("/tmp/x"))); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + private ExprNodeGenericFuncDesc buildExpression(GenericUDF udf, ExprNodeConstantDesc constant) { + List children = new ArrayList<>(); + children.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "ds", "tab", true)); + children.add(constant); + return new ExprNodeGenericFuncDesc(TypeInfoFactory.booleanTypeInfo, udf, children); + } +} From 96256538a188863e82a646df6f0f8431800e272a Mon Sep 17 00:00:00 2001 From: okumin Date: Wed, 26 Aug 2026 23:41:40 +0900 Subject: [PATCH 2/8] Improve coverage --- .../hive/ql/exec/SerializationUtilities.java | 34 ++++++++-------- .../ppr/PartitionExpressionForMetastore.java | 13 +++---- .../hive/metastore/TestMetastoreExpr.java | 14 ++++++- .../ql/exec/TestSerializationUtilities.java | 39 +++++++++++++++---- 4 files changed, 66 insertions(+), 34 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java index 2cf8c67c2997..388ab1d913b0 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java @@ -242,13 +242,11 @@ public Configuration getConf() { @Override public com.esotericsoftware.kryo.kryo5.Registration getRegistration(Class type) { - // If this instance deserializes a payload that a remote client controls (e.g. - // PartitionExpressionForMetastore at a remote HMS) or that a client can persist (e.g. a - // table property copied into the job conf), the first class encountered during - // deserialization must be compatible with the expected root type, and every class in the - // stream must pass the allowlist check. Kryo is otherwise willing to instantiate any - // classpath class named by the payload (registrationRequired=false plus - // StdInstantiatorStrategy), which turns these payloads into a + // If this instance deserializes a payload that a remote client controls (e.g. PartitionExpressionForMetastore at + // a remote HMS) or that a client can persist (e.g. a table property copied into the job conf), the first class + // encountered during deserialization must be compatible with the expected root type, and every class in the + // stream must pass the allowlist check. Kryo is otherwise willing to instantiate any classpath class named by the + // payload (registrationRequired=false plus StdInstantiatorStrategy), which turns these payloads into a // deserialization-of-untrusted-data primitive. if (untrustedRootType != null) { if (classCounter == 0 && !untrustedRootType.isAssignableFrom(type)) { @@ -282,12 +280,11 @@ public void restore() { } /** - * Package prefixes that classes read from an untrusted Kryo payload may come from. These cover - * everything a legitimate serialized expression ({@link ExprNodeDesc} graph) or search argument - * (SearchArgumentImpl graph) contains: expression descriptors and plan literals, builtin and - * installed UDFs, type infos and object inspectors, Hive/Hadoop value types, and plain JDK - * value/collection classes. Known gadget carriers (commons-collections, beanutils, - * xalan/TemplatesImpl, ...) all live outside these prefixes. + * Package prefixes that classes read from an untrusted Kryo payload may come from. These cover everything a + * legitimate serialized expression ({@link ExprNodeDesc} graph) or search argument (SearchArgumentImpl graph) + * contains: expression descriptors and plan literals, builtin and installed UDFs, type infos and object inspectors, + * Hive/Hadoop value types, and plain JDK value/collection classes. Known gadget carriers (commons-collections, + * beanutils, xalan/TemplatesImpl, ...) all live outside these prefixes. */ private static final String[] UNTRUSTED_ALLOWED_PACKAGE_PREFIXES = new String[] { "java.lang.", @@ -304,14 +301,15 @@ public void restore() { }; /** - * Classes that are never acceptable in an untrusted payload even though they pass the package - * allowlist: reflect()/reflect2() invoke arbitrary methods on arbitrary classes, so a - * pre-instantiated instance arriving in a client-supplied expression is an - * arbitrary-code-execution primitive for whoever evaluates the expression. + * Classes that are never acceptable in an untrusted payload even though they pass the package allowlist. + * GenericUDFReflect, GenericUDFReflect2, and GenericUDFInFile are typically disallowed in a secure environment. + * {@link org.apache.hadoop.hive.ql.security.authorization.plugin.SettableConfigUpdater} */ private static final Set UNTRUSTED_DENIED_CLASS_NAMES = new HashSet<>(Arrays.asList( "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect", - "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2")); + "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2", + "org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile" + )); @VisibleForTesting static boolean isAllowedForUntrustedDeserialization(Class type) { diff --git a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java index 6e21348fca4b..d7e283c29411 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java @@ -117,8 +117,8 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes) throws MetaException { } catch (Exception ex) { LOG.error("Failed to deserialize the expression, fall back to deserializeUntrustedObjectFromKryo", ex); try { - // The fallback must use the same untrusted-payload restrictions as the primary path: - // these bytes come straight from a Thrift client. + // The fallback must use the same untrusted-payload restrictions as the primary path: these bytes come straight + // from a Thrift client. expr = SerializationUtilities.deserializeUntrustedObjectFromKryo(exprBytes, ExprNodeGenericFuncDesc.class); } catch (Exception e) { LOG.error("Failed to deserialize the expression", e); @@ -134,11 +134,10 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes) throws MetaException { } /** - * Rejects client-supplied expression graphs that would execute arbitrary code when the - * metastore stringifies or evaluates them. The Kryo-level class allowlist already blocks - * reflect()/reflect2(); a {@link GenericUDFBridge} instance is legitimate (it wraps builtin - * old-style UDFs like year()), but it instantiates whatever class name its - * {@code udfClassName} field carries, so that name must resolve to a real {@link UDF}. + * Rejects client-supplied expression graphs that would execute arbitrary code when the metastore stringifies or + * evaluates them. The Kryo-level class allowlist already blocks reflect/reflect2/java_method/in_file; a + * {@link GenericUDFBridge} instance is legitimate (it wraps builtin old-style UDFs like year()), but it instantiates + * whatever class name its {@code udfClassName} field carries, so that name must resolve to a real {@link UDF}. */ private void validateDeserializedExpr(ExprNodeDesc expr) throws MetaException { if (expr instanceof ExprNodeGenericFuncDesc) { diff --git a/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java b/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java index fab397c52063..22909e4407cd 100644 --- a/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java +++ b/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java @@ -55,7 +55,7 @@ import com.google.common.collect.Lists; - +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.junit.Assert.assertEquals; import org.junit.Before; @@ -170,6 +170,18 @@ public void testPartitionExpr() throws Exception { } catch (IMetaStoreClient.IncompatibleMetastoreException ignore) { } + // Denied expression => throw the specific exception + try { + var expr = e.val("currentTimeMillis").val("java.lang.System").fn("reflect", TypeInfoFactory.intTypeInfo, 2).val(0) + .pred("=", 2).build(); + checkExpr(-1, dbName, tblName, expr, tbl); + fail("Should have thrown"); + } catch (IMetaStoreClient.IncompatibleMetastoreException ex) { + assertTrue(ex.getMessage().startsWith("SerializationUtilities#deserializeObjectWithTypeInformation: " + + "java.lang.UnsupportedOperationException: Deserialization of " + + "class org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect is not allowed from an untrusted payload")); + } + // Invalid expression => throw some exception, but not incompatible metastore. try { checkExpr(-1, dbName, tblName, e.val(31).intCol("p3").pred(">", 2).build(), tbl); diff --git a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java index 6b72db1bcd98..dc2033f6a76d 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java +++ b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java @@ -31,6 +31,7 @@ import java.util.Optional; import java.util.Properties; +import com.esotericsoftware.kryo.kryo5.KryoException; import com.google.common.collect.ArrayListMultimap; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; @@ -46,9 +47,11 @@ import org.apache.hadoop.hive.ql.plan.PartitionDesc; import org.apache.hadoop.hive.ql.plan.TableDesc; import org.apache.hadoop.hive.ql.plan.VectorPartitionDesc; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPEqual; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPNull; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2; import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory; import org.junit.Assert; import org.junit.Test; @@ -250,7 +253,7 @@ private static MapWork mockMapWorkWithSomePartitionDescProperties() throws Excep } @Test - public void testUntrustedDeserializationAcceptsLegitimateExpression() throws Exception { + public void testUntrustedDeserializationAcceptsLegitimateExpression() { ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( TypeInfoFactory.stringTypeInfo, "value")); @@ -263,14 +266,14 @@ public void testUntrustedDeserializationAcceptsLegitimateExpression() throws Exc } @Test(expected = UnsupportedOperationException.class) - public void testUntrustedDeserializationRejectsNonExprRoot() throws Exception { + public void testUntrustedDeserializationRejectsNonExprRoot() { byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation( new LinkedHashMap()); SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); } - @Test(expected = UnsupportedOperationException.class) - public void testUntrustedDeserializationRejectsSmuggledClass() throws Exception { + @Test(expected = KryoException.class) + public void testUntrustedDeserializationRejectsSmuggledClass() { // a class outside the allowlist carried in a "constant" stands in for a gadget object ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( TypeInfoFactory.stringTypeInfo, new java.io.File("/tmp/x"))); @@ -278,15 +281,15 @@ public void testUntrustedDeserializationRejectsSmuggledClass() throws Exception SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); } - @Test(expected = UnsupportedOperationException.class) - public void testDeserializeExpressionRejectsSmuggledClass() throws Exception { + @Test(expected = KryoException.class) + public void testDeserializeExpressionRejectsSmuggledClass() { ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( TypeInfoFactory.stringTypeInfo, new java.io.File("/tmp/x"))); SerializationUtilities.deserializeExpression(SerializationUtilities.serializeExpression(expr)); } - @Test(expected = UnsupportedOperationException.class) - public void testUntrustedDeserializationRejectsReflectUdf() throws Exception { + @Test(expected = KryoException.class) + public void testUntrustedDeserializationRejectsReflectUdf() { List children = new ArrayList<>(); children.add(new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); ExprNodeGenericFuncDesc expr = new ExprNodeGenericFuncDesc(TypeInfoFactory.stringTypeInfo, @@ -295,6 +298,26 @@ public void testUntrustedDeserializationRejectsReflectUdf() throws Exception { SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); } + @Test(expected = KryoException.class) + public void testUntrustedDeserializationRejectsReflect2Udf() { + List children = new ArrayList<>(); + children.add(new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); + ExprNodeGenericFuncDesc expr = new ExprNodeGenericFuncDesc(TypeInfoFactory.stringTypeInfo, + new GenericUDFReflect2(), children); + byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(expr); + SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); + } + + @Test(expected = KryoException.class) + public void testUntrustedDeserializationRejectsInFileUdf() { + List children = new ArrayList<>(); + children.add(new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); + ExprNodeGenericFuncDesc expr = new ExprNodeGenericFuncDesc(TypeInfoFactory.stringTypeInfo, + new GenericUDFInFile(), children); + byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(expr); + SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); + } + @Test public void testUntrustedDeserializationAllowlist() { Assert.assertTrue( From b82da1d741b768dc9c27885a649eac2957c51375 Mon Sep 17 00:00:00 2001 From: okumin Date: Thu, 27 Aug 2026 09:57:29 +0900 Subject: [PATCH 3/8] Address checkstyle violations --- .../ppr/PartitionExpressionForMetastore.java | 46 ++++++++++--------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java index d7e283c29411..0c834f35a2a6 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java @@ -140,27 +140,8 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes) throws MetaException { * whatever class name its {@code udfClassName} field carries, so that name must resolve to a real {@link UDF}. */ private void validateDeserializedExpr(ExprNodeDesc expr) throws MetaException { - if (expr instanceof ExprNodeGenericFuncDesc) { - GenericUDF genericUDF = ((ExprNodeGenericFuncDesc) expr).getGenericUDF(); - if (genericUDF instanceof GenericUDFBridge) { - String udfClassName = ((GenericUDFBridge) genericUDF).getUdfClassName(); - Class udfClass; - try { - udfClass = Class.forName(udfClassName, false, Thread.currentThread().getContextClassLoader()); - } catch (ClassNotFoundException | LinkageError e) { - throw new MetaException("Unknown UDF class in partition filter expression: " + udfClassName); - } - if (!UDF.class.isAssignableFrom(udfClass)) { - throw new MetaException("Class in partition filter expression is not a UDF: " + udfClassName); - } - } - if (genericUDF instanceof GenericUDFMacro) { - // a macro body is an expression graph of its own - ExprNodeDesc body = ((GenericUDFMacro) genericUDF).getBody(); - if (body != null) { - validateDeserializedExpr(body); - } - } + if (expr instanceof ExprNodeGenericFuncDesc exprNodeGenericFuncDesc) { + validateDeserializedExprNodeGenericFuncDesc(exprNodeGenericFuncDesc); } if (expr.getChildren() != null) { for (ExprNodeDesc child : expr.getChildren()) { @@ -169,6 +150,29 @@ private void validateDeserializedExpr(ExprNodeDesc expr) throws MetaException { } } + private void validateDeserializedExprNodeGenericFuncDesc(ExprNodeGenericFuncDesc expr) throws MetaException { + GenericUDF genericUDF = expr.getGenericUDF(); + if (genericUDF instanceof GenericUDFBridge genericUDFBridge) { + String udfClassName = genericUDFBridge.getUdfClassName(); + Class udfClass; + try { + udfClass = Class.forName(udfClassName, false, Thread.currentThread().getContextClassLoader()); + } catch (ClassNotFoundException | LinkageError e) { + throw new MetaException("Unknown UDF class in partition filter expression: " + udfClassName); + } + if (!UDF.class.isAssignableFrom(udfClass)) { + throw new MetaException("Class in partition filter expression is not a UDF: " + udfClassName); + } + } + if (genericUDF instanceof GenericUDFMacro genericUDFMacro) { + // a macro body is an expression graph of its own + ExprNodeDesc body = genericUDFMacro.getBody(); + if (body != null) { + validateDeserializedExpr(body); + } + } + } + @Override public FileFormatProxy getFileFormatProxy(FileMetadataExprType type) { switch (type) { From 4284a85d861d7ac0b062bf8e3f954c17eb9405b8 Mon Sep 17 00:00:00 2001 From: okumin Date: Sat, 5 Sep 2026 23:17:28 +0900 Subject: [PATCH 4/8] Complete JavaDoc --- .../org/apache/hadoop/hive/ql/exec/SerializationUtilities.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java index 388ab1d913b0..0f1d8d87fc16 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java @@ -303,7 +303,8 @@ public void restore() { /** * Classes that are never acceptable in an untrusted payload even though they pass the package allowlist. * GenericUDFReflect, GenericUDFReflect2, and GenericUDFInFile are typically disallowed in a secure environment. - * {@link org.apache.hadoop.hive.ql.security.authorization.plugin.SettableConfigUpdater} + * This set should be in sync with the denylist in + * {@link org.apache.hadoop.hive.ql.security.authorization.plugin.SettableConfigUpdater}. */ private static final Set UNTRUSTED_DENIED_CLASS_NAMES = new HashSet<>(Arrays.asList( "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect", From a3f36865cb6cda09d0f57c656dfebcc769809a25 Mon Sep 17 00:00:00 2001 From: okumin Date: Wed, 9 Sep 2026 23:24:30 -0700 Subject: [PATCH 5/8] Use getUdfClass instead of getUdfClassName --- .../optimizer/ppr/PartitionExpressionForMetastore.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java index 0c834f35a2a6..39825c6e8ef5 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java @@ -153,15 +153,9 @@ private void validateDeserializedExpr(ExprNodeDesc expr) throws MetaException { private void validateDeserializedExprNodeGenericFuncDesc(ExprNodeGenericFuncDesc expr) throws MetaException { GenericUDF genericUDF = expr.getGenericUDF(); if (genericUDF instanceof GenericUDFBridge genericUDFBridge) { - String udfClassName = genericUDFBridge.getUdfClassName(); - Class udfClass; - try { - udfClass = Class.forName(udfClassName, false, Thread.currentThread().getContextClassLoader()); - } catch (ClassNotFoundException | LinkageError e) { - throw new MetaException("Unknown UDF class in partition filter expression: " + udfClassName); - } + Class udfClass = genericUDFBridge.getUdfClass(); if (!UDF.class.isAssignableFrom(udfClass)) { - throw new MetaException("Class in partition filter expression is not a UDF: " + udfClassName); + throw new MetaException("Class in partition filter expression is not a UDF: " + udfClass); } } if (genericUDF instanceof GenericUDFMacro genericUDFMacro) { From a1581133550708e0d0eb02fb3581387a31dcef10 Mon Sep 17 00:00:00 2001 From: okumin Date: Sat, 12 Sep 2026 22:20:28 -0700 Subject: [PATCH 6/8] Disallow custom UDFs --- .../apache/hadoop/hive/ql/exec/SerializationUtilities.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java index 0f1d8d87fc16..660ce76928d3 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java @@ -60,7 +60,6 @@ import org.apache.hadoop.hive.ql.plan.MapredWork; import org.apache.hadoop.hive.ql.plan.PartitionDesc; import org.apache.hadoop.hive.ql.session.SessionState; -import org.apache.hadoop.hive.ql.udf.generic.GenericUDF; import org.apache.hadoop.hive.serde2.Serializer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -325,12 +324,6 @@ static boolean isAllowedForUntrustedDeserialization(Class type) { if (UNTRUSTED_DENIED_CLASS_NAMES.contains(name)) { return false; } - // Custom (temporary/permanent) UDFs live in user packages. The classes themselves were - // installed by an administrator, so allowing kryo to instantiate them is no worse than any - // query invoking them. - if (GenericUDF.class.isAssignableFrom(component) || UDF.class.isAssignableFrom(component)) { - return true; - } for (String prefix : UNTRUSTED_ALLOWED_PACKAGE_PREFIXES) { if (name.startsWith(prefix)) { return true; From c073621f86eeda7c7df04d61feb0a808462ef8f7 Mon Sep 17 00:00:00 2001 From: okumin Date: Tue, 22 Sep 2026 18:46:07 +0900 Subject: [PATCH 7/8] Move UDF validation to PartitionExpressionForMetastore --- .../hive/ql/exec/SerializationUtilities.java | 128 +++--------------- .../ppr/PartitionExpressionForMetastore.java | 39 ++++-- .../hive/metastore/TestMetastoreExpr.java | 8 +- .../ql/exec/TestSerializationUtilities.java | 62 --------- .../TestPartitionExpressionForMetastore.java | 52 ++++++- 5 files changed, 93 insertions(+), 196 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java index 660ce76928d3..cf341fdcbad3 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java @@ -32,14 +32,12 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Properties; -import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -128,10 +126,8 @@ private static class KryoWithHooks extends Kryo implements Configurable { private Hook globalHook; // this should be set on-the-fly after borrowing this instance and needs to be reset on release private Configuration configuration; - // when non-null, the stream being deserialized is untrusted: the first class read must be - // compatible with this type and every class read must pass - // isAllowedForUntrustedDeserialization(); default null (trusted), reset on release - private Class untrustedRootType = null; + // when non-null, the first class read must be compatible with this type + private Class rootType = null; // total classes we have met during (de)serialization, should be reset on release private long classCounter = 0; @@ -243,95 +239,28 @@ public Configuration getConf() { public com.esotericsoftware.kryo.kryo5.Registration getRegistration(Class type) { // If this instance deserializes a payload that a remote client controls (e.g. PartitionExpressionForMetastore at // a remote HMS) or that a client can persist (e.g. a table property copied into the job conf), the first class - // encountered during deserialization must be compatible with the expected root type, and every class in the - // stream must pass the allowlist check. Kryo is otherwise willing to instantiate any classpath class named by the - // payload (registrationRequired=false plus StdInstantiatorStrategy), which turns these payloads into a - // deserialization-of-untrusted-data primitive. - if (untrustedRootType != null) { - if (classCounter == 0 && !untrustedRootType.isAssignableFrom(type)) { - throw new UnsupportedOperationException("The object to be deserialized must be a " - + untrustedRootType.getName() + ", but encountered: " + type); - } - if (!isAllowedForUntrustedDeserialization(type)) { - throw new UnsupportedOperationException( - "Deserialization of " + type + " is not allowed from an untrusted payload"); - } + // encountered during deserialization must be compatible with the expected root type. + if (rootType != null && classCounter == 0 && !rootType.isAssignableFrom(type)) { + throw new UnsupportedOperationException("The object to be deserialized must be a " + + rootType.getName() + ", but encountered: " + type); } classCounter++; return super.getRegistration(type); } - public void setExprNodeFirst(boolean isPartFilter) { - setUntrustedRootType(isPartFilter ? ExprNodeDesc.class : null); - } - - void setUntrustedRootType(Class rootType) { - this.untrustedRootType = rootType; + void setRootType(Class rootType) { + this.rootType = rootType; this.classCounter = 0; } // reset the fields on release public void restore() { setConf(null); - untrustedRootType = null; + rootType = null; classCounter = 0; } } - /** - * Package prefixes that classes read from an untrusted Kryo payload may come from. These cover everything a - * legitimate serialized expression ({@link ExprNodeDesc} graph) or search argument (SearchArgumentImpl graph) - * contains: expression descriptors and plan literals, builtin and installed UDFs, type infos and object inspectors, - * Hive/Hadoop value types, and plain JDK value/collection classes. Known gadget carriers (commons-collections, - * beanutils, xalan/TemplatesImpl, ...) all live outside these prefixes. - */ - private static final String[] UNTRUSTED_ALLOWED_PACKAGE_PREFIXES = new String[] { - "java.lang.", - "java.util.", - "java.sql.", - "java.time.", - "java.math.", - "org.apache.hadoop.hive.ql.plan.", - "org.apache.hadoop.hive.ql.udf.", - "org.apache.hadoop.hive.ql.io.sarg.", - "org.apache.hadoop.hive.serde2.", - "org.apache.hadoop.hive.common.type.", - "org.apache.hadoop.io." - }; - - /** - * Classes that are never acceptable in an untrusted payload even though they pass the package allowlist. - * GenericUDFReflect, GenericUDFReflect2, and GenericUDFInFile are typically disallowed in a secure environment. - * This set should be in sync with the denylist in - * {@link org.apache.hadoop.hive.ql.security.authorization.plugin.SettableConfigUpdater}. - */ - private static final Set UNTRUSTED_DENIED_CLASS_NAMES = new HashSet<>(Arrays.asList( - "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect", - "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2", - "org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile" - )); - - @VisibleForTesting - static boolean isAllowedForUntrustedDeserialization(Class type) { - Class component = type; - while (component.isArray()) { - component = component.getComponentType(); - } - if (component.isPrimitive()) { - return true; - } - String name = component.getName(); - if (UNTRUSTED_DENIED_CLASS_NAMES.contains(name)) { - return false; - } - for (String prefix : UNTRUSTED_ALLOWED_PACKAGE_PREFIXES) { - if (name.startsWith(prefix)) { - return true; - } - } - return false; - } - private static final Object FAKE_REFERENCE = new Object(); // Bounded queue could be specified here but that will lead to blocking. @@ -938,7 +867,7 @@ public static byte[] serializeObjectWithTypeInformation(Serializable object) { public static T deserializeObjectWithTypeInformation(byte[] bytes, boolean isPartFilter) { KryoWithHooks kryo = (KryoWithHooks) borrowKryo(); - kryo.setExprNodeFirst(isPartFilter); + kryo.setRootType(isPartFilter ? ExprNodeDesc.class : null); try (Input inp = new Input(new ByteArrayInputStream(bytes))) { return (T) kryo.readClassAndObject(inp); } finally { @@ -953,29 +882,7 @@ public static String serializeExpression(ExprNodeGenericFuncDesc expr) { public static ExprNodeGenericFuncDesc deserializeExpression(String s) { byte[] bytes = Base64.decodeBase64(s.getBytes(StandardCharsets.UTF_8)); - // Serialized expressions travel through configuration values (e.g. - // hive.io.filter.expr.serialized) that clients can shadow via table properties or SET, so - // they must always be deserialized with the untrusted-payload restrictions. - return deserializeUntrustedObjectFromKryo(bytes, ExprNodeGenericFuncDesc.class); - } - - /** - * Deserializes bytes that a client may control (a remote-supplied expression, a value read back - * from a configuration key that table properties can shadow, ...). In addition to pinning the - * root object to {@code clazz}, every class named in the stream is validated against a fixed - * allowlist, so the payload cannot make Kryo instantiate arbitrary classpath classes. - * @param bytes Bytes containing the object. - * @param clazz The expected class of the root object. - * @return The deserialized object. - */ - public static T deserializeUntrustedObjectFromKryo(byte[] bytes, Class clazz) { - KryoWithHooks kryo = (KryoWithHooks) borrowKryo(); - kryo.setUntrustedRootType(clazz); - try (Input inp = new Input(new ByteArrayInputStream(bytes))) { - return kryo.readObject(inp, clazz); - } finally { - releaseKryo(kryo); - } + return deserializeObjectFromKryo(bytes, ExprNodeGenericFuncDesc.class); } public static byte[] serializeObjectToKryo(Serializable object) { @@ -991,17 +898,14 @@ public static byte[] serializeObjectToKryo(Serializable object) { return baos.toByteArray(); } - public static T deserializeObjectFromKryo(byte[] bytes, Class clazz) { - Input inp = new Input(new ByteArrayInputStream(bytes)); - Kryo kryo = borrowKryo(); - T func = null; - try { - func = kryo.readObject(inp, clazz); + public static T deserializeObjectFromKryo(byte[] bytes, Class clazz) { + KryoWithHooks kryo = (KryoWithHooks) borrowKryo(); + kryo.setRootType(clazz); + try (Input inp = new Input(new ByteArrayInputStream(bytes))) { + return kryo.readObject(inp, clazz); } finally { releaseKryo(kryo); } - inp.close(); - return func; } public static String serializeObject(Serializable expr) { diff --git a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java index 39825c6e8ef5..ff952a391f5d 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionExpressionForMetastore.java @@ -25,10 +25,12 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; +import java.util.Set; import org.apache.hadoop.hive.metastore.FileFormatProxy; import org.apache.hadoop.hive.metastore.PartitionExpressionProxy; import org.apache.hadoop.hive.metastore.api.MetaException; +import org.apache.hadoop.hive.ql.exec.FunctionRegistry; import org.apache.hadoop.hive.ql.exec.SerializationUtilities; import org.apache.hadoop.hive.ql.exec.UDF; import org.apache.hadoop.hive.ql.io.orc.OrcFileFormatProxy; @@ -42,7 +44,10 @@ import org.apache.hadoop.hive.ql.plan.ExprNodeGenericFuncDesc; import org.apache.hadoop.hive.ql.udf.generic.GenericUDF; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFBridge; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFMacro; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2; import org.apache.hadoop.hive.serde2.typeinfo.PrimitiveTypeInfo; import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory; import org.slf4j.Logger; @@ -54,6 +59,18 @@ public class PartitionExpressionForMetastore implements PartitionExpressionProxy { private static final Logger LOG = LoggerFactory.getLogger(PartitionExpressionForMetastore.class); + /** + * Classes that are never acceptable in a partition expression. + * GenericUDFReflect, GenericUDFReflect2, and GenericUDFInFile are typically disallowed in a secure environment. + * This set should be in sync with the denylist in + * {@link org.apache.hadoop.hive.ql.security.authorization.plugin.SettableConfigUpdater}. + */ + private static final Set> DENIED_UDFS = Set.of( + GenericUDFReflect.class, + GenericUDFReflect2.class, + GenericUDFInFile.class + ); + @Override public String convertExprToFilter(byte[] exprBytes, String defaultPartitionName, boolean decodeFilterExpToStr) throws MetaException { @@ -115,15 +132,13 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes) throws MetaException { try { expr = SerializationUtilities.deserializeObjectWithTypeInformation(exprBytes, true); } catch (Exception ex) { - LOG.error("Failed to deserialize the expression, fall back to deserializeUntrustedObjectFromKryo", ex); + LOG.error("Failed to deserialize the expression, fall back to deserializeObjectFromKryo", ex); try { - // The fallback must use the same untrusted-payload restrictions as the primary path: these bytes come straight - // from a Thrift client. - expr = SerializationUtilities.deserializeUntrustedObjectFromKryo(exprBytes, ExprNodeGenericFuncDesc.class); + expr = SerializationUtilities.deserializeObjectFromKryo(exprBytes, ExprNodeGenericFuncDesc.class); } catch (Exception e) { LOG.error("Failed to deserialize the expression", e); throw new MetaException("SerializationUtilities#deserializeObjectWithTypeInformation: " + ex.getMessage() + - ", SerializationUtilities#deserializeUntrustedObjectFromKryo: " + e.getMessage()); + ", SerializationUtilities#deserializeObjectFromKryo: " + e.getMessage()); } } if (expr == null) { @@ -135,9 +150,7 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes) throws MetaException { /** * Rejects client-supplied expression graphs that would execute arbitrary code when the metastore stringifies or - * evaluates them. The Kryo-level class allowlist already blocks reflect/reflect2/java_method/in_file; a - * {@link GenericUDFBridge} instance is legitimate (it wraps builtin old-style UDFs like year()), but it instantiates - * whatever class name its {@code udfClassName} field carries, so that name must resolve to a real {@link UDF}. + * evaluates them. */ private void validateDeserializedExpr(ExprNodeDesc expr) throws MetaException { if (expr instanceof ExprNodeGenericFuncDesc exprNodeGenericFuncDesc) { @@ -152,6 +165,12 @@ private void validateDeserializedExpr(ExprNodeDesc expr) throws MetaException { private void validateDeserializedExprNodeGenericFuncDesc(ExprNodeGenericFuncDesc expr) throws MetaException { GenericUDF genericUDF = expr.getGenericUDF(); + if (DENIED_UDFS.contains(genericUDF.getClass())) { + throw new MetaException(genericUDF.getUdfName() + " is not allowed in partition expressions"); + } + if (!FunctionRegistry.isBuiltInFuncExpr(expr)) { + throw new MetaException("Only built-in UDFs are allowed in partition expressions"); + } if (genericUDF instanceof GenericUDFBridge genericUDFBridge) { Class udfClass = genericUDFBridge.getUdfClass(); if (!UDF.class.isAssignableFrom(udfClass)) { @@ -191,8 +210,6 @@ public FileMetadataExprType getMetadataType(String inputFormat) { @Override public SearchArgument createSarg(byte[] expr) { - // These bytes also come straight from a Thrift client (get_file_metadata_by_expr), so they - // get the same untrusted-payload restrictions as the partition filter expressions above. - return SerializationUtilities.deserializeUntrustedObjectFromKryo(expr, SearchArgumentImpl.class); + return SerializationUtilities.deserializeObjectFromKryo(expr, SearchArgumentImpl.class); } } diff --git a/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java b/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java index 22909e4407cd..8eb1f8563ae1 100644 --- a/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java +++ b/ql/src/test/org/apache/hadoop/hive/metastore/TestMetastoreExpr.java @@ -55,7 +55,6 @@ import com.google.common.collect.Lists; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.junit.Assert.assertEquals; import org.junit.Before; @@ -177,9 +176,10 @@ public void testPartitionExpr() throws Exception { checkExpr(-1, dbName, tblName, expr, tbl); fail("Should have thrown"); } catch (IMetaStoreClient.IncompatibleMetastoreException ex) { - assertTrue(ex.getMessage().startsWith("SerializationUtilities#deserializeObjectWithTypeInformation: " + - "java.lang.UnsupportedOperationException: Deserialization of " + - "class org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect is not allowed from an untrusted payload")); + assertEquals( + "org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect is not allowed in partition expressions", + ex.getMessage() + ); } // Invalid expression => throw some exception, but not incompatible metastore. diff --git a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java index dc2033f6a76d..891ee7ca022a 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java +++ b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java @@ -31,7 +31,6 @@ import java.util.Optional; import java.util.Properties; -import com.esotericsoftware.kryo.kryo5.KryoException; import com.google.common.collect.ArrayListMultimap; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; @@ -47,11 +46,8 @@ import org.apache.hadoop.hive.ql.plan.PartitionDesc; import org.apache.hadoop.hive.ql.plan.TableDesc; import org.apache.hadoop.hive.ql.plan.VectorPartitionDesc; -import org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPEqual; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPNull; -import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect; -import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2; import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory; import org.junit.Assert; import org.junit.Test; @@ -272,64 +268,6 @@ public void testUntrustedDeserializationRejectsNonExprRoot() { SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); } - @Test(expected = KryoException.class) - public void testUntrustedDeserializationRejectsSmuggledClass() { - // a class outside the allowlist carried in a "constant" stands in for a gadget object - ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( - TypeInfoFactory.stringTypeInfo, new java.io.File("/tmp/x"))); - byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(expr); - SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); - } - - @Test(expected = KryoException.class) - public void testDeserializeExpressionRejectsSmuggledClass() { - ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( - TypeInfoFactory.stringTypeInfo, new java.io.File("/tmp/x"))); - SerializationUtilities.deserializeExpression(SerializationUtilities.serializeExpression(expr)); - } - - @Test(expected = KryoException.class) - public void testUntrustedDeserializationRejectsReflectUdf() { - List children = new ArrayList<>(); - children.add(new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); - ExprNodeGenericFuncDesc expr = new ExprNodeGenericFuncDesc(TypeInfoFactory.stringTypeInfo, - new GenericUDFReflect(), children); - byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(expr); - SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); - } - - @Test(expected = KryoException.class) - public void testUntrustedDeserializationRejectsReflect2Udf() { - List children = new ArrayList<>(); - children.add(new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); - ExprNodeGenericFuncDesc expr = new ExprNodeGenericFuncDesc(TypeInfoFactory.stringTypeInfo, - new GenericUDFReflect2(), children); - byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(expr); - SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); - } - - @Test(expected = KryoException.class) - public void testUntrustedDeserializationRejectsInFileUdf() { - List children = new ArrayList<>(); - children.add(new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); - ExprNodeGenericFuncDesc expr = new ExprNodeGenericFuncDesc(TypeInfoFactory.stringTypeInfo, - new GenericUDFInFile(), children); - byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(expr); - SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); - } - - @Test - public void testUntrustedDeserializationAllowlist() { - Assert.assertTrue( - SerializationUtilities.isAllowedForUntrustedDeserialization(ExprNodeGenericFuncDesc.class)); - Assert.assertTrue(SerializationUtilities.isAllowedForUntrustedDeserialization(GenericUDFOPNull.class)); - Assert.assertTrue(SerializationUtilities.isAllowedForUntrustedDeserialization(ArrayList.class)); - Assert.assertTrue(SerializationUtilities.isAllowedForUntrustedDeserialization(byte[].class)); - Assert.assertFalse(SerializationUtilities.isAllowedForUntrustedDeserialization(java.io.File.class)); - Assert.assertFalse(SerializationUtilities.isAllowedForUntrustedDeserialization(Path.class)); - Assert.assertFalse(SerializationUtilities.isAllowedForUntrustedDeserialization(GenericUDFReflect.class)); - } - private static ExprNodeGenericFuncDesc buildColumnEqualsConstant(ExprNodeConstantDesc constant) { List children = new ArrayList<>(); children.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "col1", "tab", false)); diff --git a/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java index 21358bb08170..4bc4fe2e8ef4 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java +++ b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java @@ -22,14 +22,20 @@ import org.apache.hadoop.hive.metastore.api.MetaException; import org.apache.hadoop.hive.ql.exec.SerializationUtilities; +import org.apache.hadoop.hive.ql.exec.UDFArgumentException; +import org.apache.hadoop.hive.ql.metadata.HiveException; import org.apache.hadoop.hive.ql.plan.ExprNodeColumnDesc; import org.apache.hadoop.hive.ql.plan.ExprNodeConstantDesc; import org.apache.hadoop.hive.ql.plan.ExprNodeDesc; import org.apache.hadoop.hive.ql.plan.ExprNodeGenericFuncDesc; import org.apache.hadoop.hive.ql.udf.generic.GenericUDF; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFBridge; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFInFile; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFOPEqual; import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect; +import org.apache.hadoop.hive.ql.udf.generic.GenericUDFReflect2; +import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector; +import org.apache.hadoop.hive.serde2.objectinspector.primitive.PrimitiveObjectInspectorFactory; import org.apache.hadoop.hive.serde2.typeinfo.TypeInfoFactory; import org.junit.Assert; import org.junit.Test; @@ -41,6 +47,22 @@ * must all be rejected before anything stringifies or evaluates the expression. */ public class TestPartitionExpressionForMetastore { + public static class CustomGenericUDF extends GenericUDF { + @Override + public ObjectInspector initialize(ObjectInspector[] arguments) throws UDFArgumentException { + return PrimitiveObjectInspectorFactory.writableVoidObjectInspector; + } + + @Override + public Object evaluate(DeferredObject[] arguments) throws HiveException { + return null; + } + + @Override + public String getDisplayString(String[] children) { + return "custom_udf"; + } + } @Test public void testComparisonExpressionIsAccepted() throws Exception { @@ -60,18 +82,34 @@ public void testReflectUdfIsRejected() throws Exception { } @Test(expected = MetaException.class) - public void testBridgeToNonUdfClassIsRejected() throws Exception { - GenericUDFBridge bridge = new GenericUDFBridge("evil", false, "java.lang.ProcessBuilder"); - ExprNodeGenericFuncDesc expr = buildExpression(bridge, - new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "x")); + public void testReflect2UdfIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFReflect2(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "java.lang.ProcessBuilder")); new PartitionExpressionForMetastore().convertExprToFilter( SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); } @Test(expected = MetaException.class) - public void testSmuggledClassIsRejected() throws Exception { - ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFOPEqual(), - new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, new java.io.File("/tmp/x"))); + public void testInFileUdfIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new GenericUDFInFile(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "/tmp/x")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + @Test(expected = MetaException.class) + public void testCustomUdfIsRejected() throws Exception { + ExprNodeGenericFuncDesc expr = buildExpression(new CustomGenericUDF(), + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "test")); + new PartitionExpressionForMetastore().convertExprToFilter( + SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); + } + + @Test(expected = MetaException.class) + public void testBridgeToNonUdfClassIsRejected() throws Exception { + GenericUDFBridge bridge = new GenericUDFBridge("evil", false, "java.lang.ProcessBuilder"); + ExprNodeGenericFuncDesc expr = buildExpression(bridge, + new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "x")); new PartitionExpressionForMetastore().convertExprToFilter( SerializationUtilities.serializeObjectWithTypeInformation(expr), null, false); } From 1acadd6aa5e1d2441afb5152323f1d40c59dbf92 Mon Sep 17 00:00:00 2001 From: okumin Date: Wed, 23 Sep 2026 13:08:25 +0900 Subject: [PATCH 8/8] Update unit tests --- .../ql/exec/TestSerializationUtilities.java | 26 ++++++++++++++++--- .../TestPartitionExpressionForMetastore.java | 11 ++++---- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java index 891ee7ca022a..eef5891f2cce 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java +++ b/ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java @@ -53,7 +53,6 @@ import org.junit.Test; public class TestSerializationUtilities { - @Test public void testEveryPropertiesAreSerialized() throws Exception { MapWork mapWork = doSerDeser(null); @@ -249,7 +248,7 @@ private static MapWork mockMapWorkWithSomePartitionDescProperties() throws Excep } @Test - public void testUntrustedDeserializationAcceptsLegitimateExpression() { + public void testDeserializeObjectWithTypeInformationAcceptsLegitimateExpression() { ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( TypeInfoFactory.stringTypeInfo, "value")); @@ -262,12 +261,33 @@ public void testUntrustedDeserializationAcceptsLegitimateExpression() { } @Test(expected = UnsupportedOperationException.class) - public void testUntrustedDeserializationRejectsNonExprRoot() { + public void testDeserializeObjectWithTypeInformationRejectsNonExprRoot() { byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation( new LinkedHashMap()); SerializationUtilities.deserializeObjectWithTypeInformation(bytes, true); } + @Test + public void testDeserializeObjectFromKryoAcceptsLegitimateExpression() { + ExprNodeGenericFuncDesc expr = buildColumnEqualsConstant(new ExprNodeConstantDesc( + TypeInfoFactory.stringTypeInfo, "value")); + String exprString = "(col1 = 'value')"; + Assert.assertEquals(exprString, expr.getExprString()); + + byte[] kryo = SerializationUtilities.serializeObjectToKryo(expr); + Assert.assertEquals( + exprString, + SerializationUtilities.deserializeObjectFromKryo(kryo, ExprNodeGenericFuncDesc.class).getExprString() + ); + Assert.assertNotNull(SerializationUtilities.deserializeObjectFromKryo(kryo, Object.class)); + + String base64 = SerializationUtilities.serializeExpression(expr); + Assert.assertEquals( + exprString, + SerializationUtilities.deserializeExpression(base64).getExprString() + ); + } + private static ExprNodeGenericFuncDesc buildColumnEqualsConstant(ExprNodeConstantDesc constant) { List children = new ArrayList<>(); children.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "col1", "tab", false)); diff --git a/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java index 4bc4fe2e8ef4..a074a599eb8f 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java +++ b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java @@ -9,11 +9,12 @@ * * 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. + * 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.hive.ql.optimizer.ppr;