nishat-06 commented on code in PR #8689:
URL: https://github.com/apache/hadoop/pull/8689#discussion_r4190444224
##########
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:
Agreed, the wildcard is gone. I generated NUMA records with each release's
own `AssignedResources.toBytes()` and its hadoop-shaded-guava: 3.3.1, 3.3.6,
3.4.0 through 3.4.3 and 3.5.0 (shaded-guava 1.1.1 through 1.5.0). Every release
writes byte-identical records, and the only guava types in them are
`ImmutableBiMap$SerializedForm` (single-entry maps, the single-node allocation
path) and `ImmutableMap$SerializedForm` (multi-entry and empty maps), plus the
`Object[]` carrying the keys and values. Those two names are now the whole
guava allowlist.
Both records are checked in as fixtures in
`testFromBytesReadsNumaRecordsFromPriorReleases`, and
`testFromBytesRejectsGuavaTypesOutsideTheAllowlist` covers your two examples: a
shaded `ImmutableList` as the record itself, and as an element inside an
allowed `ArrayList`. Pre-shading records (3.3.0 and earlier) aren't readable by
any current NM regardless of this patch, since `NumaResourceAllocation`'s
fields are now the shaded `ImmutableMap`; I confirmed a 3.3.0-generated record
fails on unpatched trunk with a ClassCastException.
Since the proxy names have held across five shaded-guava releases and the
fixtures will flag any drift, I kept the enumeration rather than introducing a
new format.
--
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]
