fix: ClassCastException in JSON.toJSON() when a generic field's cached initWriter does not match the runtime value class, for issue #7853 - #7854
Conversation
…cached initWriter does not match the runtime value class, for issue alibaba#7853
|
|
wenshao
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6c": 未编译 base 侧 ObjectWriterAdapter.java 做改前/改后字节码大小对比(head 侧偏移已证明两侧同为 >325,无跨越,故未继续)。; "agent 6c": 未运行 Issue7853.java 本身(需 JUnit 平台与 mvn ,brief 禁止在共享 worktree 内构建),改用等价探针复现了同一调用序列。; "agent 1d": 无(未因预算上限中止任何检查)。.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if (valueWriter instanceof ObjectWriterAdapter) { | ||
| ObjectWriterAdapter objectWriterAdapter = (ObjectWriterAdapter) valueWriter; |
There was a problem hiding this comment.
[Suggestion] R1-4: The reverse cache-poisoning order is still unfixed, and it fails quietly. The new guard lives inside the valueWriter instanceof ObjectWriterAdapter branch, so when the cached initWriter is a scalar writer and the current value is a bean, the branch is skipped entirely and fieldValue reaches jsonObject.put (line 701) with no conversion at all. Concretely: run this PR's own test blocks in the opposite order - serialize ResponseResult<Boolean> first, which CAS-caches the Boolean writer as the result field's initObjectWriter, then call JSON.toJSON on a ResponseResult<PagerDataBean<String>>. The returned string looks perfectly correct, but the value under result is still a raw PagerDataBean rather than a JSONObject, so the first (JSONObject) json.get("result") cast in the caller throws ClassCastException - the same exception class issue 7853 exists to remove, arriving through the other ordering. Issue7853.java covers bean-then-scalar only, so nothing in the suite observes this direction.
Witness:
ARM=pr pager toJSONString = {"code":0,"msg":"ok","result":{"rows":["x"],"total":1}}
pager toJSON = {"code":0,"msg":"ok","result":{"rows":["x"],"total":1}} <- string looks correct
result runtime class = probe.P14$PagerDataBean
result instanceof JSONObject = false
CAST THREW java.lang.ClassCastException:
probe.P14$PagerDataBean cannot be cast to com.alibaba.fastjson2.JSONObject
ARM=base byte-identical to pr (a gap in the fix, not a regression)
The cache pair was also observed directly:
initValueClass = class PagerDataBean / initObjectWriter = OWG_2_2_PagerDataBean
Moving the match decision outside the instanceof branch would cover both orderings with one predicate - and keying it on FieldWriterObject.initValueClass (the class CAS-written as the partner of initObjectWriter) rather than on the writer's own objectClass covers the scalar-writer case too, which objectClass cannot express:
ObjectWriter valueWriter = fieldWriter.getInitWriter();
if (valueWriter != null) {
boolean reuse;
if (valueWriter instanceof ObjectWriterAdapter) {
ObjectWriterAdapter objectWriterAdapter = (ObjectWriterAdapter) valueWriter;
reuse = objectWriterAdapter.objectClass != null
&& objectWriterAdapter.objectClass.isAssignableFrom(fieldValue.getClass())
&& !objectWriterAdapter.getFieldWriters().isEmpty();
} else {
reuse = false; // a scalar writer cannot convert a bean value
}
fieldValue = reuse
? ((ObjectWriterAdapter) valueWriter).toJSONObject(fieldValue)
: JSON.toJSON(fieldValue);
}Any fix keyed on initValueClass must handle null and preserve the write-once semantics: FieldWriterObject.java:27 declares volatile Class initValueClass; and it is CAS-written together with initObjectWriter only while null (FieldWriterObject.java:154-167), so it records only the FIRST observed value class.
Please add the Boolean-first ordering to Issue7853.java with assertInstanceOf(JSONObject.class, ((JSONObject) JSON.toJSON(pagerResult)).get("result")), then remove the new reuse = false arm, re-run it, and confirm it reds - the existing bean-first ordering stays green either way, so it cannot pin this.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // Only reuse it when the runtime value class matches, otherwise | ||
| // convert the value generically to avoid a ClassCastException. |
There was a problem hiding this comment.
[Suggestion] R1-3: toJSONObject has two fieldWriter.getInitWriter() cache-reuse sites and only this one received the guard. The UNWRAPPED_MASK branch 50 lines above (lines 636-653) still hands the stale cached writer's field writers a value of an unrelated class, so a user who reaches issue 7853 through an @JSONField(unwrapped = true) wrapper field gets no relief from this patch. Serialize a ResponseResult<PagerDataBean<String>> first - the write path CAS-caches the PagerDataBean adapter as the unwrapped field's initObjectWriter - then call JSON.toJSON on a ResponseResult<Boolean>: it throws JSONException: field.get error, rows while JSON.toJSONString of the very same object succeeds. There is a quieter variant of the same site that produces wrong JSON with no exception at all: with @JSONField(unwrapped = true) Iface data and two implementations, JSON.toJSON returns A's key names carrying B's values. core/src/test/.../features/UnwrappedTest.java contains no toJSON/toJSONObject call, so the branch has no coverage. This is pre-existing (the base arm is byte-identical); it is raised here because the diff chose to guard one member of the pair and not the other.
Witness:
ARM=pr bool toJSONString = {"code":0,"msg":"ok","result":true}
bool toJSON THREW com.alibaba.fastjson2.JSONException: field.get error, rows
at FieldWriter.errorOnGet(FieldWriter.java:424)
at FieldWriterList.getFieldValue(FieldWriter.java:426)
at ObjectWriterAdapter.toJSONObject(ObjectWriterAdapter.java:649)
ARM=pr unwrapped interface + two implementations:
h2 toJSONString = {"code":0,"y":2}
h2 toJSON = {"code":0,"x":2} <- A's key names carrying B's value, no exception
ARM=base both byte-identical to pr
The cleanest fix is to share one predicate between the two sites rather than copy this inline expression, which the sibling cannot reuse - e.g. add ObjectWriter getInitWriter(Class valueClass) on FieldWriter (base implementation return getInitWriter();), override it in FieldWriterObject to return the cached writer only when it was resolved for valueClass, and call fieldWriter.getInitWriter(fieldValue.getClass()) at both line 642 and line 684:
// ObjectWriterAdapter.java:642, in the UNWRAPPED_MASK branch
ObjectWriter fieldObjectWriter = fieldWriter.getInitWriter(fieldValue == null ? null : fieldValue.getClass());A guard added in that branch must not call getClass() unconditionally: lines 636-653 sit BEFORE the null handling at line 655 (if (fieldValue != null)) and line 670 (if (fieldValue == null && ... WriteNulls.mask ...) continue;), so fieldValue can legitimately be null there - unlike this site, which is protected by the fieldValue != null condition at line 683.
Please add a test with an @JSONField(unwrapped = true) field of a generic/Object/interface declared type serialized with two unrelated value classes in sequence, asserting JSON.toJSON(...) returns the flattened object instead of throwing; then remove the shared guard, re-run it, and confirm it reds with JSONException: field.get error, rows.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // a later call serializes ResponseResult<Boolean>). | ||
| // Only reuse it when the runtime value class matches, otherwise | ||
| // convert the value generically to avoid a ClassCastException. | ||
| if (objectWriterAdapter.objectClass.isAssignableFrom(fieldValue.getClass()) |
There was a problem hiding this comment.
[Suggestion] R1-2: This predicate is looser than the rule the library already applies to this very same cached writer. FieldWriterObject.typeMatch requires exact class equality and widens only for writeUsing / Map / List, whereas isAssignableFrom accepts any subclass - so a superclass writer is reused for a subclass value and the subclass-only fields are silently dropped. With Holder{Object value}, Base{String a} and Sub extends Base{String b}: serialize the Base instance first so the field CAS-caches initValueClass=Base / initObjectWriter=adapter(Base), then serialize a Sub instance. JSON.toJSON returns {"value":{"a":"A"}} while JSON.toJSONString returns {"value":{"a":"A","b":"B"}} - field b is gone with no error, and which shape you get depends on what some earlier, unrelated serialization happened to cache, so a caller that persists or forwards the toJSON result stores a truncated snapshot. The dropping itself predates this diff (the base arm is byte-identical - the old code reused unconditionally), but this diff rewrites the line that decides it, and the comment above it claims "Only reuse it when the runtime value class matches", which isAssignableFrom does not implement.
Witness:
ARM=pr toJSONString(h2) = {"value":{"a":"A","b":"B"}}
toJSON(h2) = {"value":{"a":"A"}} AGREE=false 'b' present=false
ARM=fixB (objectClass == fieldValue.getClass())
toJSON(h2) = {"value":{"a":"A","b":"B"}} AGREE=true 'b' present=true
the PR's own Issue7853 six assertions ALL PASS, and the R1-1 NPE disappears too
ARM=base byte-identical to pr (AGREE=false)
Mirroring the write path's rule closes the divergence - and note that exact equality also retires the null-objectClass NPE reported separately, because null == fieldValue.getClass() is false and the value falls to the existing JSON.toJSON(fieldValue) branch (measured above as ARM=fixB). Left as a plain block rather than a one-click suggestion because it overlaps the range of that other comment:
Class valueClass = fieldValue.getClass();
boolean typeMatch = objectWriterAdapter.objectClass == valueClass
|| (((FieldWriterObject) fieldWriter).writeUsing
&& objectWriterAdapter.objectClass.isAssignableFrom(valueClass));
if (typeMatch && !objectWriterAdapter.getFieldWriters().isEmpty()) {A tightened predicate must keep the writeUsing arm: FieldWriterObject.java:82-85 reads boolean typeMatch = initValueClass == valueClass || (writeUsing && initValueClass.isAssignableFrom(valueClass)) || (initValueClass == Map.class && initValueClass.isAssignableFrom(valueClass)) || (initValueClass == List.class && initValueClass.isAssignableFrom(valueClass));, so dropping it would bypass a user writer registered for a supertype via @JSONField(writeUsing = ...) and diverge the two APIs in the opposite direction. Two more facts a fix must respect: initValueClass/initObjectWriter are written once by compareAndSet(this, null, ...) (FieldWriterObject.java:156-167), so a mismatch is a stable state rather than a transient race; and ObjectWriterBaseModule.java:1579 builds the Field.class writer as new ObjectWriterAdapter<>(Method.class, ...), so an exact-match rule must rely on the else branch re-resolving by runtime class rather than assuming objectClass is the target class (measured: it does, and output is unchanged).
Please add a Base/Sub pair to Issue7853.java (Sub adding a field) that primes the cache with JSON.toJSONString on the Base instance and then asserts JSON.toJSON(subHolder).toString() equals JSON.toJSONString(subHolder) and contains the subclass field; then revert the predicate to isAssignableFrom, re-run it, and confirm it reds - the existing three blocks cannot discriminate the two predicates, since for PagerDataBean / Boolean / PagerDataBean both give the same answer.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if (objectWriterAdapter.objectClass.isAssignableFrom(fieldValue.getClass()) | ||
| && !objectWriterAdapter.getFieldWriters().isEmpty() | ||
| ) { |
There was a problem hiding this comment.
[Critical] R1-1: [fails-closed] [regression] This guard dereferences objectWriterAdapter.objectClass unconditionally, but a null objectClass is a legally constructed state of ObjectWriterAdapter, so JSON.toJSON now throws a bare NullPointerException where the merge base returned a JSONObject. The public no-class factories all funnel into ObjectWriterCreator.java:92 (new ObjectWriterAdapter(null, null, null, 0, fieldWriters)), producing an adapter with objectClass == null and a non-empty fieldWriters list, and ObjectWriterCreator.createFieldWriter (:1299-1310) installs such a writer as the initObjectWriter of any bean field of that type. So an application that registers one - including via JSON.register(Long.class, ObjectWriters.ofToString(Object::toString)), the exact pattern this repo's own core/src/test/java/com/alibaba/fastjson2/issues_3900/Issue3932.java:20 uses - and then calls JSON.toJSON on a bean holding that type now crashes on every such call, with no message and a stack that points at a line whose neighbouring comment is about generic arguments. JSONObject.java:340, JSONArray.java:213 and the fastjson1-compatible JSONObject/JSONArray all reach this line, so every toJSON entry point is affected. The condition replaced here never touched objectClass, which makes this a regression rather than a latent gap.
Witness:
A/B probe. Arm proof first - count of isAssignableFrom calls inside toJSONObject(T,long): pr=2, base=1
ARM=pr toJSONString = {"id":"o1","price":"$1234"}
toJSON THREW java.lang.NullPointerException
at ObjectWriterAdapter.toJSONObject(ObjectWriterAdapter.java:696)
at JSON.toJSON(JSON.java:4084) <- JSON.toJSON(JSON.java:4056)
ARM=base toJSON = {"id":"o1","price":{"toString":"$1234"}}
Second arm, Issue3932.java:20 verbatim:
JSON.register(Long.class, ObjectWriters.ofToString(Object::toString))
ARM=pr toJSONString = {"id":123,"name":"n"}
toJSON THREW java.lang.NullPointerException (same stack)
ARM=base toJSON = {"id":{"toString":"123"},"name":"n"}
Sensitivity: with the null tolerance below applied, both arms become byte-identical to base
and the PR's own Issue7853 assertions all still pass.
Coverage sweep over the real test sources: 35 test files in core + fastjson1-compatible use a
no-class factory, and 0 of them also call toJSON( / toJSONObject( - the existing suite
structurally cannot catch this.
| if (objectWriterAdapter.objectClass.isAssignableFrom(fieldValue.getClass()) | |
| && !objectWriterAdapter.getFieldWriters().isEmpty() | |
| ) { | |
| Class writerObjectClass = objectWriterAdapter.objectClass; | |
| if ((writerObjectClass == null || writerObjectClass.isAssignableFrom(fieldValue.getClass())) | |
| && !objectWriterAdapter.getFieldWriters().isEmpty() | |
| ) { |
The fix must not assume non-null, and must not be closed by rejecting null in the constructor - ObjectWriterAdapter.java:80 already treats it as a supported state (this.serializable = objectClass == null || java.io.Serializable.class.isAssignableFrom(objectClass);, see also :65 if (typeName == null && objectClass != null)), and ObjectWriterCreator.java:92 is a public factory path reached from ObjectWriters.ofToString/ofToInt/ofToLong/objectWriter(FieldWriter...) (ObjectWriters.java:42-72). Note also that adopting exact class equality instead of isAssignableFrom (the separate comment on line 696) retires this NPE as well, so if you take that route this term becomes unnecessary - either way the null case needs a deliberate decision rather than a dereference.
No test in this diff covers the path, since Issue7853.java only exercises a non-null objectClass, so please add a case that registers ObjectWriters.ofToString(...) for a type used as a bean field and asserts JSON.toJSON(bean) returns the pre-patch JSONObject instead of throwing; then remove the writerObjectClass == null || term, re-run it, and confirm it reds with the NPE at line 696. JSON.register mutates the global default provider, so register and restore in try-finally the way Issue3932.java does.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| ) { | ||
| fieldValue = objectWriterAdapter.toJSONObject(fieldValue); | ||
| } else { | ||
| fieldValue = JSON.toJSON(fieldValue); |
There was a problem hiding this comment.
[Suggestion] R1-5: This fallback becomes the permanent route for a mismatched value, and it is materially more expensive than the direct conversion. FieldWriterObject caches with write-once compareAndSet(this, null, ...), so once a generic wrapper's field is bound to one type argument's adapter it stays bound for the process lifetime - the guard never repairs the binding, it just diverts around it forever. Every later JSON.toJSON(new ResponseResult<Boolean>()) on a per-request path then allocates a fresh JSONWriter.Context via JSONFactory.createWriteContext() (JSONFactory.java:461-463), and because ObjectWriterImplBoolean is not an ObjectWriterAdapter it falls through JSONWriter.of -> write -> toString() -> parse(str) (JSON.java:4088-4096) - a writer, a char buffer, a String and a JSONReader allocated to emit true. The write() path in the same mismatch situation does one provider cache lookup (FieldWriterObject.java:184-201) with no Context and no round trip. To be explicit about scale: this is not a regression, because pre-patch this path threw, and the full String round trip applies only when the runtime writer is not an adapter - for bean values JSON.toJSON internally takes the adapter fast path and the measured net overhead is +24.6 ns/op (1.15x). But for the scalar case, which is exactly this PR's own ResponseResult<Boolean> scenario, the round trip is unconditional and permanent, in a project whose AGENTS.md states performance-first.
Witness:
A counting writer makes the round-trip count observable 1:1.
ARM=pr call 1 countingWriter.write() = 1, call 2 = 2, call 3 = 3
total = 220003 (3 + 20000 warmup + 200000 timed, i.e. exactly one round trip per call)
mismatched-value fallback : 793.1 ns/op
ARM=pr cache state after toJSON(ResponseResult<Boolean>):
initValueClass / initObjectWriter are STILL PagerDataBean -> the cache is never repaired
ARM=pr fair A/B (same value class, same output, same work):
matched = {"value":{"n":1,"x":"x"}} mismatch = {"value":{"n":1,"x":"x"}} same output = true
matched (direct toJSONObject) : 168.8 ns/op
mismatch (JSON.toJSON fallback) : 193.4 ns/op
fallback overhead : +24.6 ns/op (1.15x)
ARM=base every call throws JSONException: field.get error, rows (corroborates "not a regression")
Resolving the writer by the runtime class keeps the direct conversion for bean values and hands only genuinely non-adapter values to JSON.toJSON:
} else {
ObjectWriter runtimeWriter = JSONFactory.getDefaultObjectWriterProvider()
.getObjectWriter(fieldValue.getClass());
if (runtimeWriter instanceof ObjectWriterAdapter
&& !((ObjectWriterAdapter) runtimeWriter).getFieldWriters().isEmpty()
&& (runtimeWriter.getFeatures() & JSONWriter.Feature.WriteClassName.mask) == 0) {
fieldValue = ((ObjectWriterAdapter) runtimeWriter).toJSONObject(fieldValue);
} else {
fieldValue = JSON.toJSON(fieldValue);
}
}A hand-written provider lookup here must keep both gates that JSON.toJSON itself applies - JSON.java:4081-4083 reads if (objectWriter instanceof ObjectWriterAdapter && !writeContext.isEnabled(JSONWriter.Feature.ReferenceDetection) && (objectWriter.getFeatures() & JSONWriter.Feature.WriteClassName.mask) == 0) - or output under WriteClassName / ReferenceDetection changes. Short-circuiting the scalar String round trip as well would move an output-semantics boundary, so it is worth evaluating and testing separately rather than folding into this fix.
The existing assertions 2 and 3 in Issue7853.java pin the output and must stay green through any rewrite of this branch, but they do not pin the absence of the round trip; if you take this change, please add an assertion that does (a counting ObjectWriterProvider, or asserting no extra Context/String allocation across two calls on the same mismatched value), then remove the provider-direct-lookup arm, re-run it, and confirm it reds.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| assertEquals("{\"code\":0,\"msg\":\"ok\",\"result\":true}", | ||
| JSON.toJSON(boolResult).toString()); |
There was a problem hiding this comment.
[Suggestion] R1-8: This test does not pin the guard's predicate in either direction, so the semantics the fix chose are entirely unguarded. Reversing the two operands of isAssignableFrom - writing fieldValue.getClass().isAssignableFrom(objectWriterAdapter.objectClass) - keeps the whole core suite green, and so does replacing isAssignableFrom with exact class equality, and so does forcing the else branch always. The surviving mutants are not harmless: with a field declared as the supertype, serializing the subclass first (which caches the subclass writer) and then the superclass makes JSON.toJSON read the superclass object through the subclass writer's field offsets and throw, and in the whole-file revert probe the same Unsafe misread took the JDK 8 forked JVM to SIGSEGV. So whoever later flips or loosens this predicate gets a green CI while users get an exception or a crashed JVM. For contrast, the hunk itself IS gated - reverting the whole file to the merge base with this test kept makes it SIGSEGV twice - it is only the predicate's direction and tightness that nothing observes.
Witness:
Deterministic [test] source - measured, not inferred.
./mvnw -pl core clean test on the operand-reversal mutant:
Tests run: 7989, Failures: 0, Errors: 0, Skipped: 0 - BUILD SUCCESS
Issue7853: Tests run: 1, Failures: 0
Same mutant, standalone probe (field declared as the supertype, subclass serialized first):
toJSON(animal) THREW java.lang.NullPointerException
at PropertyAccessorFactoryUnsafe$FieldAccessorUnsafeString.getString(:502)
<- FieldWriterString.getFieldValue <- ObjectWriterAdapter.toJSONObject(:618/:699)
toJSONString(animal) = {"value":{"name":"a"}}
Whole-file revert to merge base 879183e94 with this test kept:
forked VM SIGSEGV twice (Process Exit Code: 134,
Crashed tests: com.alibaba.fastjson2.issues_7800.Issue7853)
Unmutated baselines: ./mvnw -pl core clean test -> 7989/0/0 BUILD SUCCESS (2:50);
./mvnw -pl fastjson1-compatible test -> 1355/0/0 BUILD SUCCESS (1:16);
./mvnw -pl core validate -> 0 Checkstyle violations.
One extra case is enough to pin the direction - prime the cache with the subclass, then convert the superclass:
public static class Animal {
public String name = "a";
}
public static class Dog extends Animal {
public String breed = "b";
}
public static class Holder {
public Object value;
}
@Test
public void testGuardDirection() {
Holder dogHolder = new Holder();
dogHolder.value = new Dog();
JSON.toJSONString(dogHolder); // caches the Dog writer on the "value" field
Holder animalHolder = new Holder();
animalHolder.value = new Animal();
// with the operands reversed this reads Animal through Dog's field offsets and throws
assertEquals("{\"value\":{\"name\":\"a\"}}", JSON.toJSON(animalHolder).toString());
assertEquals("{\"value\":{\"name\":\"a\"}}", JSON.toJSONString(animalHolder));
}Whichever predicate is finally adopted on line 696, the assertion here must not contradict the write path's rule at FieldWriterObject.java:82-83 (boolean typeMatch = initValueClass == valueClass || (writeUsing && initValueClass.isAssignableFrom(valueClass))).
Please confirm the mutation yourself: add the case above, reverse the two operands of isAssignableFrom, re-run Issue7853, and check that the new case reds while the existing three blocks stay green - that is the gate which is missing today.
— qwen3.8-max via Qwen Code /review (v0.23.0)
What this PR does / why we need it?
Fixes #7853.
When serializing the same generic bean class with different generic arguments via
JSON.toJSON(), aClassCastExceptionis thrown (e.g.Boolean cannot be cast to PagerDataBean), becauseObjectWriterAdapter.toJSONObjectreuses the writer cached byfieldWriter.getInitWriter()without checking that it matches the runtime value class.Note that on some JDK versions this bug can even manifest as a JVM crash (
EXCEPTION_ACCESS_VIOLATION) instead of a cleanClassCastException, since the mismatched value reaches ASM-generated primitive accessors.Summary of your change
In
ObjectWriterAdapter.toJSONObject(core), thegetInitWriter()cached writer is now only reused whenobjectWriterAdapter.objectClass.isAssignableFrom(fieldValue.getClass()). Otherwise the value is converted generically viaJSON.toJSON(fieldValue)— the same behavior thetoJSONStringpath already has via thetypeMatchcheck inFieldWriterObject.getObjectWriter.ObjectWriterAdapter.java: type-match guard before reusing the cached init writer.Issue7853.java(new test): coversJSON.toJSON/JSON.toJSONStringfor the same generic wrapper serialized first withPagerDataBean<String>and then withBooleanandPagerDataBean<Integer>.Reproduction before the fix (JDK 17):
After the fix:
{"code":0,"msg":"ok","result":true}— identical to thetoJSONStringoutput.Please indicate you've done the following: