search

FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
316 stars 136 forks source link

`AvroEncodeTest` failed under non-deterministic order #305

Closed shunfan-shao closed 2 years ago

shunfan-shao commented 3 years ago

Description

As for our course project, we used the tool called NonDex to detect flakiness of code that relies on non-deterministic data structures. We found vulnerability in test com.fasterxml.jackson.dataformat.avro.interop.annotations.AvroEncodeTest in branch 2.14.

Reproduce

One should be able to reproduce the bug after install NonDex tool. 

mvn edu.illinois:nondex-maven-plugin:1.1.2:nondex \
     -pl avro \
     -Dtest=com.fasterxml.jackson.dataformat.avro.interop.annotations.AvroEncodeTest

Issue

Here is a copy of the trace if I ran with edu.illinois:nondex-maven-plugin:1.1.2:debug.

java.lang.Class.getDeclaredFields(Class.java:1916)
org.apache.avro.reflect.ReflectData.getFields(ReflectData.java:716)
org.apache.avro.reflect.ReflectData.getCachedFields(ReflectData.java:703)
org.apache.avro.reflect.ReflectData.createSchema(ReflectData.java:601)
com.fasterxml.jackson.dataformat.avro.interop.ApacheAvroInteropUtil$7.createSchema(ApacheAvroInteropUtil.java:149)
...

Here getDeclaredFields is non-deterministic and is the root cause. Stepping into the dependencies, avro module has a dependency of org.apache.avro with version 1.8.2.

Notice that the usage of getDeclaredFields does not guarantee the order of fields traversal.

for (Field field : c.getDeclaredFields())

In avro 1.10.2, they have fixed the issues by sorting the fields: 

Field[] declaredFields = c.getDeclaredFields();
Arrays.sort(declaredFields, Comparator.comparing(Field::getName));
for (Field field : declaredFields)

If I just changed the dependency in pom.xml, the above issue will be resolved. However, the newer version of dependency of avro does not seem to be compatible and will cause failures of other tests.

I have not be able to find an easier approach to the issue. Please let me know if anything is unclear.

cowtowncoder commented 2 years ago

Unfortunately I do not see how we could actually reproduce the failure here. I don't doubt that the order in which Java class fields are exposed can vary across platforms, but that is quite far removed from the test in question. So I'd need a reproduction to see how things could break.

I will close this for now; issue may be reopened with a reproduction or more specific explanation of changes within Jackson component (possibly a PR showing proposed changes).