Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,15 @@

package org.apache.hadoop.yarn.server.nodemanager.containermanager.container;

import org.apache.commons.io.serialization.ValidatingObjectInputStream;
import org.apache.commons.lang3.SerializationException;
import org.apache.commons.lang3.SerializationUtils;
import org.apache.hadoop.yarn.server.nodemanager.api.deviceplugin.Device;
import org.apache.hadoop.yarn.server.nodemanager.containermanager.linux.resources.numa.NumaResourceAllocation;
import org.apache.hadoop.yarn.server.nodemanager.containermanager.resourceplugin.fpga.FpgaDevice;
import org.apache.hadoop.yarn.server.nodemanager.containermanager.resourceplugin.gpu.GpuDevice;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.Serializable;
import java.util.ArrayList;
Expand Down Expand Up @@ -89,9 +95,37 @@ 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.
try (ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
ValidatingObjectInputStream ois =
new ValidatingObjectInputStream(bais)) {
// The concrete value objects the resource plugins store, the list
// types that wrap them (a fresh ArrayList, or the unmodifiable view
// that legacy records were serialized from), and the strings / boxed
// primitives those objects hold. Number is the superclass descriptor
// read back for the boxed Integer / Long map values.
ois.accept(ArrayList.class, String.class, Number.class,
Integer.class, Long.class, Device.class, GpuDevice.class,
FpgaDevice.class, NumaResourceAllocation.class);
ois.accept("java.util.Collections$UnmodifiableList",
"java.util.Collections$UnmodifiableCollection");
// NumaResourceAllocation serializes its shaded-guava ImmutableMaps
// through guava's internal SerializedForm, which carries the keys and
// values in an Object[]. The forms are a guava-internal detail, so the
// collect package is matched by pattern rather than pinned class name.
ois.accept(
"org.apache.hadoop.thirdparty.com.google.common.collect.*",
"[Ljava.lang.Object;");
Comment on lines +119 to +121

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remaining package wildcard still weakens the stated known types boundary.

For example, a shaded-Guava ImmutableList and its serialization proxy match this pattern and can be accepted even though they are not assigned-resource value types. Likewise, any serializable class in this package can be instantiated as an element of an allowed ArrayList before the top-level instanceof List check.

Could we enumerate the exact ImmutableMap serialization proxy types required by the current and supported upgrade paths, and cover those paths with compatibility fixtures? If those internal names cannot be made sufficiently stable, a versioned non-Java-serialization format may be safer than retaining a package-wide wildcard.

Object obj = ois.readObject();
if (!(obj instanceof List)) {
throw new IOException("Unexpected assigned-resources record type: "
+ (obj == null ? "null" : obj.getClass().getName()));
}
resources = (List<Serializable>) obj;
} catch (ClassNotFoundException e) {
throw new IOException(e);
}
AssignedResources ar = new AssignedResources();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,21 @@

import org.apache.hadoop.thirdparty.com.google.common.collect.ImmutableList;
import org.apache.hadoop.yarn.server.nodemanager.api.deviceplugin.Device;
import org.apache.hadoop.yarn.server.nodemanager.containermanager.linux.resources.numa.NumaResourceAllocation;
import org.apache.hadoop.yarn.server.nodemanager.containermanager.resourceplugin.fpga.FpgaDevice;
import org.apache.hadoop.yarn.server.nodemanager.containermanager.resourceplugin.gpu.GpuDevice;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.util.List;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.fail;

public class TestResourceMappings {
Expand Down Expand Up @@ -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 object
// is instantiated and any of its logic runs.
byte[] payload = toBytes(ImmutableList.<Serializable>of(
new File("/etc/passwd")));
assertThrows(IOException.class,
() -> ResourceMappings.AssignedResources.fromBytes(payload));
}

/**
* This was the legacy way to serialize resources. This is here for
* backward compatibility to ensure that after YARN-9128 we can still
Expand Down
Loading