nishat-06 commented on code in PR #8689:
URL: https://github.com/apache/hadoop/pull/8689#discussion_r3819611527
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/container/ResourceMappings.java:
##########
@@ -89,9 +91,24 @@ public void updateAssignedResources(List<Serializable> list)
{
public static AssignedResources fromBytes(byte[] bytes)
throws IOException {
final List<Serializable> resources;
- try {
- resources = SerializationUtils.deserialize(bytes);
- } catch (SerializationException e) {
+ // The bytes come from the NM recovery state store and are read back
+ // during container recovery on restart. Deserialize through a
+ // ValidatingObjectInputStream so a tampered record cannot instantiate
+ // arbitrary serializable classes on the NodeManager classpath. The
+ // allowed graph is the assigned-resource value objects the resource
+ // plugins store (device / NUMA descriptors, plain strings) plus the
+ // collection types that wrap them.
+ try (ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
+ ValidatingObjectInputStream ois =
+ new ValidatingObjectInputStream(bais)) {
+ ois.accept(
+ "org.apache.hadoop.yarn.server.nodemanager.*",
+ "org.apache.hadoop.thirdparty.com.google.common.collect.*",
+ "java.util.*",
+ "java.lang.*",
+ "[Ljava.lang.Object;");
Review Comment:
Good point. Tightened it: the four package wildcards are gone, replaced with
the concrete value objects the plugins actually store (Device, GpuDevice,
FpgaDevice, NumaResourceAllocation), the ArrayList/UnmodifiableList that wrap
them, and the String/Number/Integer/Long those objects hold. The only remaining
wildcard is the shaded-guava collect package, because NumaResourceAllocation's
ImmutableMaps serialize through guava's internal SerializedForm and pinning
those class names would break across guava versions.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/container/ResourceMappings.java:
##########
@@ -89,9 +91,24 @@ public void updateAssignedResources(List<Serializable> list)
{
public static AssignedResources fromBytes(byte[] bytes)
throws IOException {
final List<Serializable> resources;
- try {
- resources = SerializationUtils.deserialize(bytes);
- } catch (SerializationException e) {
+ // The bytes come from the NM recovery state store and are read back
+ // during container recovery on restart. Deserialize through a
+ // ValidatingObjectInputStream so a tampered record cannot instantiate
+ // arbitrary serializable classes on the NodeManager classpath. The
+ // allowed graph is the assigned-resource value objects the resource
+ // plugins store (device / NUMA descriptors, plain strings) plus the
+ // collection types that wrap them.
+ try (ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
+ ValidatingObjectInputStream ois =
+ new ValidatingObjectInputStream(bais)) {
+ ois.accept(
+ "org.apache.hadoop.yarn.server.nodemanager.*",
+ "org.apache.hadoop.thirdparty.com.google.common.collect.*",
+ "java.util.*",
+ "java.lang.*",
+ "[Ljava.lang.Object;");
+ resources = (List<Serializable>) ois.readObject();
Review Comment:
Fixed. It now reads into an Object, checks instanceof List and throws an
IOException with the offending type name if it isn't one, so the cast can't
produce a stray ClassCastException. Callers see the checked IOException the
contract advertises.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/container/TestResourceMappings.java:
##########
@@ -94,6 +99,38 @@ public void
testAssignedResourcesCanDeserializePreviouslySerializedValues() {
}
}
+ @Test
+ public void testRoundTripCoversResourcePluginTypes() throws IOException {
+ // The elements a NodeManager actually stores for gpu / fpga / numa
+ // resources must survive the allowlist, otherwise recovery would break.
+ ResourceMappings.AssignedResources pluginResources =
+ new ResourceMappings.AssignedResources();
+ pluginResources.updateAssignedResources(ImmutableList.of(
+ new GpuDevice(2, 3),
+ new FpgaDevice("IntelOpenCL", 247, 0, "aclv0"),
+ new NumaResourceAllocation("0", 1024L, "0", 4),
+ "cpu-0"));
+
+ ResourceMappings.AssignedResources deserialized =
+
ResourceMappings.AssignedResources.fromBytes(pluginResources.toBytes());
+
+ assertEquals(pluginResources.getAssignedResources(),
+ deserialized.getAssignedResources());
+ }
+
+ @Test
+ public void testFromBytesRejectsUnexpectedType() throws IOException {
+ // A tampered record whose top-level list is fine but which carries an
+ // element of a type the resource plugins never store. This stands in for a
+ // serialization gadget (e.g. a commons-beanutils BeanComparator): the
+ // allowlist rejects it by class name during readObject, before the class
+ // is loaded or any of its logic runs.
Review Comment:
Reworded to 'before the object is instantiated and any of its logic runs',
which is the property the allowlist actually guarantees.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
