[GitHub] [spark] fsamuel-bs commented on a change in pull request #33879: [SPARK-36627][CORE] Fix java deserialization of proxy classes

[GitBox <gi...@apache.org>]

fsamuel-bs commented on a change in pull request #33879:
URL: https://github.com/apache/spark/pull/33879#discussion_r699528902



##########
File path: core/src/test/scala/org/apache/spark/serializer/JavaSerializerSuite.scala
##########
@@ -31,11 +31,31 @@ class JavaSerializerSuite extends SparkFunSuite {
   test("Deserialize object containing a primitive Class as attribute") {
     val serializer = new JavaSerializer(new SparkConf())
     val instance = serializer.newInstance()
-    val obj = instance.deserialize[ContainsPrimitiveClass](instance.serialize(
-      new ContainsPrimitiveClass()))
+    val obj = instance.deserialize[ContainsPrimitiveClass](
+      instance.serialize(new ContainsPrimitiveClass()))
     // enforce class cast
     obj.getClass
   }
+
+  test("SPARK-36627: Deserialize object containing a proxy Class as attribute") {
+    var classesLoaded = Set[String]()
+    val outer = Thread.currentThread.getContextClassLoader
+    val inner = new ClassLoader() {
+      override def loadClass(name: String): Class[_] = {
+        classesLoaded = classesLoaded + name
+        outer.loadClass(name)
+      }
+    }
+    Thread.currentThread.setContextClassLoader(inner)
+
+    val serializer = new JavaSerializer(new SparkConf())
+    val instance = serializer.newInstance()
+    val obj =
+      instance.deserialize[ContainsProxyClass](instance.serialize(new ContainsProxyClass()))
+    // enforce class cast
+    obj.getClass
+    assert(classesLoaded.exists(klass => klass.contains("MyInterface")))

Review comment:
       This is a **very** roundabout way of testing this, but I couldn't think of another way of testing it - the way we hit this issue was running spark in YARN, and I don't think we can trivially reproduce it with spark local.
   
   In order to repro exactly what happened in YARN we'd need to make `sun.misc.VM.latestUserDefinedLoader()` not be able to resolve `MyInterface`, but I couldn't figure out how to do it.

##########
File path: core/src/main/scala/org/apache/spark/serializer/JavaSerializer.scala
##########
@@ -71,6 +71,13 @@ private[spark] class JavaDeserializationStream(in: InputStream, loader: ClassLoa
         case e: ClassNotFoundException =>
           JavaDeserializationStream.primitiveMappings.getOrElse(desc.getName, throw e)
       }
+
+    override def resolveProxyClass(ifaces: Array[String]): Class[_] = {
+        // scalastyle:off classforname
+        val resolved = ifaces.map(iface => Class.forName(iface, false, loader))

Review comment:
       We definitely want better exceptions if this fails, but not sure what exactly we'd want here.




-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org