Skip to content
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

Async command serialization might fail in some cases (jdk 16) #1089

Closed
alniks opened this issue Mar 30, 2021 · 8 comments
Closed

Async command serialization might fail in some cases (jdk 16) #1089

alniks opened this issue Mar 30, 2021 · 8 comments

Comments

@alniks
Copy link

alniks commented Mar 30, 2021

Add to HelloCommand a TreeMap field:

public class HelloCommand extends Command {

    private static AtomicInteger counter = new AtomicInteger(0);
    private String message;
    private Map<String, Object> data;

    public HelloCommand(String message) {
        this.message = message;
        this.data = new TreeMap<>();
        data.put("message", message);
    }

    public HelloCommand() {} //necessary to provide

    public static void reset() {
        counter = new AtomicInteger(0);
    }

    public static int counter() {
        return counter.get();
    }

    @Override
    public void execute() {
//        try {
//            Thread.sleep(1000);
//        } catch (InterruptedException ignore) {}

        synchronized (counter){
            counter.incrementAndGet();
        }
    }

    public String getMessage() {
        return message;
    }
}

Run the CommandSpec.

Result:

Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.util.Comparator java.util.TreeMap.comparator accessible: module java.base does not "opens java.util" to unnamed module @23ceabc1
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
	at com.thoughtworks.xstream.core.util.Fields.locate(Fields.java:40)
	at com.thoughtworks.xstream.converters.collections.TreeMapConverter$Reflections.<clinit>(TreeMapConverter.java:135)

Expected result: tests are not failing.

The main problem is serializing models with xstream

@alniks
Copy link
Author

alniks commented Mar 30, 2021

respective issue in xstream: x-stream/xstream#101

@ipolevoy
Copy link
Member

The other case is when an ActiveJDBC Model is added to a Command as a member. Due to the fact that the Model attributes are type if TreeMap, we have the same issue with a Model.

The solution for Models is to not inject a model instance into a command, but rather use a Model.toJSON() or Model.toXML() and send that via a command

@ipolevoy
Copy link
Member

The solutions are:

  1. To the exception: do not use TreeMap in the Commands that you pass to Async. I changed the Model.toMap() method to return HashMap rather than TreeMap. Sending live models through the Async is probably not the brightest idea (I'm guilty of that too!). The solution now that should work is to do: Map personMap = person.toMap(), then pass the map to Async
  2. To the warning: "Security framework of XStream not initialized, XStream is probably vulnerable": I completely shut down any security in XStream with:
X_STREAM.addPermission(NoTypePermission.NONE );
X_STREAM.addPermission(AnyTypePermission.ANY);

and that got rid of the security warning. The rationale for XStream security is that a malicious attack is possible when an attacker gets to modify the XML. Once that is done, the resulting deserialized object might have injected code or data. In the context of Async, this is not relevant, since the communications is done only over JMS in a controlled environment. If an attacker gets access to your broker, you will have bigger issues to deal with :)

In other words, I do not think that XStream security is applicable to JavaLite Async

ipolevoy added a commit that referenced this issue Apr 20, 2021
#1089 - Async command serialization might fail in some cases (jdk 16)
@alniks
Copy link
Author

alniks commented Apr 28, 2021

One more option is to add a simple converter for Models:

public class ModelConverter implements Converter {

        @Override
        public void marshal(Object source, HierarchicalStreamWriter writer, MarshallingContext context) {
            Model model = (Model) source;
            context.convertAnother(model.toMap());
        }

        @Override
        public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext context) {
            try {
                Map map = (Map) context.convertAnother(context.currentObject(), Map.class);
                Class<Model> type = context.getRequiredType();
                Model instance = type.getDeclaredConstructor().newInstance();
                instance.fromMap(map);
                return instance;
            } catch (NoSuchMethodException | SecurityException | InstantiationException | IllegalAccessException | IllegalArgumentException | InvocationTargetException ex) {
                throw new UnsupportedOperationException(context.getRequiredType() + " is not supported");
            }
        }

        @Override
        public boolean canConvert(Class type) {
            return Model.class.isAssignableFrom(type);
        }
        
}

and then add it to XStream:

xStream.registerConverter(new ModelConverter());

@alniks
Copy link
Author

alniks commented Apr 28, 2021

The same could be done for CaseInsentitiveMap and some collections which are not currently supported by XStream: List.of, Arrays.asList. XStream already has some converters for collections, which could be reused:

  • TreeMapConverter
  • CollectionConverter
  • SingletonCollectionConverter

@ipolevoy
Copy link
Member

Interesting!

@ipolevoy ipolevoy reopened this Apr 28, 2021
@ipolevoy
Copy link
Member

added some converters to to process JSONMap and JSONList here: #1136. I wonder if we need to expose XStream from a command so as to allow people to add converters outside JavaLite API, @alniks ?

@alniks
Copy link
Author

alniks commented Jun 28, 2021

It's better to do on Async level - add option to Async's configuration. Multiple commands might use the same converter. And sure enough you never know what class is going to require a custom converter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants