-
Notifications
You must be signed in to change notification settings - Fork 238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Java] StackOverflowError serializing scala enumerations (Scala 2.12) #1032
Comments
Scala 2.12 error:
|
Hi @pjfanning Thanks for taking time to test and creating this issue. All scala types should be serialized correctly by fury, whether the type is used frequently or barely used. We can optimize ony for common used types, but all types should be serialized correctly. I tested your code locally, it seems scala 2.12 has circular reference for this enumeration. For shared reference, you need to enable refTracking by |
But the serialized bytes are prettly large, even 3 times smaller than JDK serialization: I haven't dive into it, but seems all state in object WeekDay extends Enumeration {
type WeekDay = Value
val Mon, Tue, Wed, Thu, Fri, Sat, Sun = Value
} @pjfanning Do you think |
@pjfanning Fury is 3x smaller than JDK surprised me, it seems fury can make a big difference for scala serialization. Is there any other scala serialization framework like fury? |
JDK test code: import io.fury.Fury
import io.fury.config.Language
import org.scalatest.matchers.should.Matchers
import org.scalatest.wordspec.AnyWordSpec
import java.io.{ByteArrayOutputStream, ObjectOutputStream}
class FuryEnumeration212Test extends AnyWordSpec with Matchers {
"fury scala enumeration support" should {
"serialize/deserialize Weekday" in {
// https://github.com/alipay/fury/issues/1032
val fury = Fury.builder().withLanguage(Language.JAVA).withRefTracking(true)
.requireClassRegistration(true).build()
fury.register(ScalaClasses.ScalaEnumerationValClass)
fury.register(Class.forName("scala.collection.mutable.HashMap"))
fury.register(WeekDay.getClass)
val bytes = fury.serialize(WeekDay.Wed)
println("fury " + bytes.length)
fury.deserialize(bytes) shouldBe WeekDay.Wed
val s = new ByteArrayOutputStream()
val s2 = new ObjectOutputStream(s)
s2.writeObject(WeekDay.Wed)
println("jdk " + s.toByteArray.length)
}
}
} |
Would it be possible to make |
We make it as default, but disabled it later. Serialization frameworks such as protobuf/flatbuffers/avro all doesn't support ref tracking, which may shows there are no shared references for most cases. Reference trakcing is not free, it need to look up a IdentifyHashMap, which will be the bottleneck if we have too much small objects. And this error happens only for serialization, not for deserialization, so the attacker can't use this to attack the systems. If there is a StackOverflowError thrown, it can be diagnosed when developing the application. Perhaps we can try catch this exception and thrown a more detailed message to tell users to enable ref tracking for such cases. |
Add detailed message and |
Describe the bug
Fury 0.2.1.
See https://github.com/pjfanning/fury-scala-test/blob/9866b728d27f167551642542b950d966f09b9b57/src/test/scala-2.12/io/github/pjfanning/fury/FuryEnumeration212Test.scala
The StackOverflowError only happens with Scala 2.12 - I have a variant on this test that works ok with Scala 2.13 and Scala 3.
Big changes happened in Scala 2.13 as part of the prep for Scala 3 - so the serialization in the more recent Scala versions needs users to register some Proxy classes as well as the expected classes.
The error happens even if I set
requireClassRegistration(false)
.Even if you don't think it is important to support Scala Enumerations for Scala 2.12 users, I think it would be useful to prevent StackOverflowErrors by having some sort of recursion depth check.
The text was updated successfully, but these errors were encountered: