Closed
Description
JavaBean:
public class Person implements Serializable {
private static final long serialVersionUID = 1L;
private String id;
private String name;
//getter, setter
}
SerializeUtil:
static Pool<Kryo> kryoPool = new Pool<Kryo>(true, false, 100) {
@Override
protected Kryo create() {
Kryo kryo = new Kryo();
UnmodifiableCollectionsSerializer.registerSerializers(kryo);
kryo.setInstantiatorStrategy(new DefaultInstantiatorStrategy(new StdInstantiatorStrategy()));
kryo.setReferences(false);
kryo.setRegistrationRequired(false);
kryo.setDefaultSerializer(CompatibleFieldSerializer.class);
return kryo;
}
};
serialize a person object to file, note that id field is now String type, after then change id to Long type, deserialize from file to a Person object, it succeed, and you can see a String value hacker was assigned to a Long field in the below picture(Line 74),
when code execute to line 77, jvm crashed, maybe I should not change id type from String to Long, but if kryo throws exception instead of crashing jvm will be more elegant.
Env:jdk8, kryo 5.0.0-RC1, kryo-serializers 0.45(I think this issue has nothing to do with kryo-serializers)
Metadata
Metadata
Assignees
Labels
No labels
Type
Projects
Milestone
Relationships
Development
No branches or pull requests
Participants
Activity
isaki commentedon Mar 24, 2019
I've never seen the JVM crash for improper payloads; normally I get a KryoException (which is an unchecked exception) under Kryo 4.0.2. Do you have a stack trace for the crash?
HiwayChe commentedon Mar 25, 2019
I'll try Kryo 4.0.2 and watch the result, also provide reproduction code and jvm crash report later this afternoon.
HiwayChe commentedon Mar 25, 2019
https://github.com/HiwayChe/kryotest
I have uploaded reproduction code in github, you can run Main.testKryo5 to reproduce the issue, and I write reproduction instruction in code comment.
Jdk:Hotspot 1.8.0
isaki commentedon Mar 25, 2019
Well that's.... alarming. Your code and instructions were spot on. I'm able to reproduce (with a few minor changes) on MacOS under Oracle JDK 8u201. I'll put together a build under OpenJDK and test there as well to see if I get the same crash.
I'll also look into your notes about Kryo4 and respond back here.
isaki commentedon Mar 25, 2019
Arch Linux (64 bit), OpenJDK-8u212 (changes included the file path moving from D: to /tmp and changing the POM to use a shaded JAR so that I had a complete class path and a main class in the manifest).
isaki commentedon Mar 25, 2019
Stack Info from Linux crash:
Process Info (Not all of it, just the interesting parts):
I can share the entire crash dump if needed.
isaki commentedon Mar 25, 2019
If you remove the CompatibleFieldSerializer declaration under Kryo 5, you get the "-48" answer, which is really, really strange (I'd have expected a KryoException be thrown here).
isaki commentedon Mar 25, 2019
4.0.2:
I get "-48" for both the default field serializer and compatible field serializer (though if I use the file generate by the compatible field serializer with the default, you get "-52"). This is with Asm enablement both true and false (consistent results).
For some reason, "hacker" is being converted to a Long of -49. I created a really long string and ended up with -1 (printing 0 due to the +1L in the test code).
isaki commentedon Mar 25, 2019
Here is the Kryo5 trace log for the crash (compatible field serializer in use):
isaki commentedon Apr 7, 2019
I have not had time to dig into this any deeper; I have not forgotten about it.
isaki commentedon Apr 16, 2019
I have a working fix. However, there are other things about the way this happens outside of compatible field serializer that bother me and I want to see if I can clean up.
Sample output/exception:
EDIT: I think throwing here is wrong; I'm going to see if I can play nice and just log the issue.
EDIT 2: Throwing is right, but only if chunked is disabled. I'm happy with what I have. Will update once I've dealt with FieldSerializer accessing memory it shouldn't (but doesn't SIGSEGV).
isaki commentedon Apr 17, 2019
For normal field serializer, you get a corrupt stream if you change the type when you have additional fields in the object. This is acceptable. I'll submit my fix for compatible mode.
isaki commentedon Apr 17, 2019
Unit Tests fail; for some odd reason primitives are getting the correct Unsafe cache field, but the value class is the boxed version which throws on my validation. It will take me a bit longer to button this up as a result; might not be done tonight.
NathanSweet commentedon May 3, 2019
Thanks @isaki for digging into the problem (both problems, the crash and the unit tests) and providing a PR. I'll commit a fix slightly different from the PR (we can fix the unit test differently and I prefer not putting issues in comments, as it adds a lot of noise over time and is easily outdated).
4 remaining items