This is an automated email from the ASF dual-hosted git repository.
saihemanth-cloudera pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hive.git
The following commit(s) were added to refs/heads/master by this push:
new f0c2c1c57dc HIVE-29838 (#6726)
f0c2c1c57dc is described below
commit f0c2c1c57dcf69d785bc3fb503511d673e4697a4
Author: Shohei Okumiya <[email protected]>
AuthorDate: Sat Sep 26 05:48:15 2026 +0900
HIVE-29838 (#6726)
* Add validations to PartitionExpressionForMetastore
Generated-by: Claude
* Improve coverage
* Address checkstyle violations
* Complete JavaDoc
* Use getUdfClass instead of getUdfClassName
* Disallow custom UDFs
* Move UDF validation to PartitionExpressionForMetastore
* Update unit tests
---
.../hive/ql/exec/SerializationUtilities.java | 40 +++----
.../ppr/PartitionExpressionForMetastore.java | 64 ++++++++++-
.../hadoop/hive/metastore/TestMetastoreExpr.java | 14 ++-
.../hive/ql/exec/TestSerializationUtilities.java | 51 ++++++++-
.../ppr/TestPartitionExpressionForMetastore.java | 124 +++++++++++++++++++++
5 files changed, 267 insertions(+), 26 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 b61f4d48443..cf341fdcbad 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
@@ -126,8 +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;
- // default false, should be reset on release
- private boolean isExprNodeFirst = false;
+ // 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;
@@ -237,27 +237,26 @@ 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)) {
- throw new UnsupportedOperationException(
- "The object to be deserialized must be an ExprNodeDesc, but
encountered: " + 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.
+ 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) {
- this.isExprNodeFirst = isPartFilter;
+ void setRootType(Class<?> rootType) {
+ this.rootType = rootType;
+ this.classCounter = 0;
}
// reset the fields on release
public void restore() {
setConf(null);
- isExprNodeFirst = false;
+ rootType = null;
classCounter = 0;
}
}
@@ -868,7 +867,7 @@ public static byte[]
serializeObjectWithTypeInformation(Serializable object) {
public static <T> 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 {
@@ -899,17 +898,14 @@ public static byte[] serializeObjectToKryo(Serializable
object) {
return baos.toByteArray();
}
- public static <T extends Serializable> T deserializeObjectFromKryo(byte[]
bytes, Class<T> clazz) {
- Input inp = new Input(new ByteArrayInputStream(bytes));
- Kryo kryo = borrowKryo();
- T func = null;
- try {
- func = kryo.readObject(inp, clazz);
+ public static <T> T deserializeObjectFromKryo(byte[] bytes, Class<T> 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 522c9896684..ff952a391f5 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,20 +25,29 @@
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;
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.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;
@@ -50,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<Class<? extends GenericUDF>> DENIED_UDFS = Set.of(
+ GenericUDFReflect.class,
+ GenericUDFReflect2.class,
+ GenericUDFInFile.class
+ );
+
@Override
public String convertExprToFilter(byte[] exprBytes, String
defaultPartitionName, boolean decodeFilterExpToStr)
throws MetaException {
@@ -123,9 +144,48 @@ private ExprNodeDesc deserializeExpr(byte[] exprBytes)
throws MetaException {
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.
+ */
+ private void validateDeserializedExpr(ExprNodeDesc expr) throws
MetaException {
+ if (expr instanceof ExprNodeGenericFuncDesc exprNodeGenericFuncDesc) {
+ validateDeserializedExprNodeGenericFuncDesc(exprNodeGenericFuncDesc);
+ }
+ if (expr.getChildren() != null) {
+ for (ExprNodeDesc child : expr.getChildren()) {
+ validateDeserializedExpr(child);
+ }
+ }
+ }
+
+ 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<? extends UDF> udfClass = genericUDFBridge.getUdfClass();
+ if (!UDF.class.isAssignableFrom(udfClass)) {
+ throw new MetaException("Class in partition filter expression is not a
UDF: " + udfClass);
+ }
+ }
+ 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) {
@@ -150,6 +210,6 @@ public FileMetadataExprType getMetadataType(String
inputFormat) {
@Override
public SearchArgument createSarg(byte[] expr) {
- return ConvertAstToSearchArg.create(expr);
+ 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 fab397c5206..8eb1f8563ae 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.fail;
import static org.junit.Assert.assertEquals;
import org.junit.Before;
@@ -170,6 +169,19 @@ 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) {
+ 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.
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 0c003d5e46d..eef5891f2cc 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,13 +46,13 @@
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.serde2.typeinfo.TypeInfoFactory;
import org.junit.Assert;
import org.junit.Test;
public class TestSerializationUtilities {
-
@Test
public void testEveryPropertiesAreSerialized() throws Exception {
MapWork mapWork = doSerDeser(null);
@@ -246,4 +246,53 @@ private static MapWork
mockMapWorkWithSomePartitionDescProperties() throws Excep
return mapWork;
}
+
+ @Test
+ public void
testDeserializeObjectWithTypeInformationAcceptsLegitimateExpression() {
+ 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 testDeserializeObjectWithTypeInformationRejectsNonExprRoot() {
+ byte[] bytes = SerializationUtilities.serializeObjectWithTypeInformation(
+ new LinkedHashMap<String, String>());
+ 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<ExprNodeDesc> 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 00000000000..a074a599eb8
--- /dev/null
+++
b/ql/src/test/org/apache/hadoop/hive/ql/optimizer/ppr/TestPartitionExpressionForMetastore.java
@@ -0,0 +1,124 @@
+/*
+ * 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.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;
+
+/**
+ * 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 {
+ 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 {
+ 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 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 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);
+ }
+
+ private ExprNodeGenericFuncDesc buildExpression(GenericUDF udf,
ExprNodeConstantDesc constant) {
+ List<ExprNodeDesc> children = new ArrayList<>();
+ children.add(new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "ds",
"tab", true));
+ children.add(constant);
+ return new ExprNodeGenericFuncDesc(TypeInfoFactory.booleanTypeInfo, udf,
children);
+ }
+}
