Erroneous type deserialization on some signals

[haggish]

I am listening to some signals from DBus, one of them is "ServicesChanged" from Connman, which has a signature of a(oa{sv})ao.

It seems that the signals that have the first array as empty, and the other array (of object paths) non-empty, result in a ClassCastException:

2018-07-19 08:17:52 [DBus Worker Thread-3] WARN  o.f.d.c.impl.DBusConnection - Exception while running signal handler 'xxx.ConfigurationService$ServicesChangeHandler@38db2f' for signal 'DBusSignal [clazz=class xxx.IDBusConnmanManager$ServicesChanged]': {}
org.freedesktop.dbus.exceptions.DBusException: java.lang.IllegalArgumentException: java.lang.ClassCastException@1858f7
	at org.freedesktop.dbus.messages.DBusSignal.createReal(DBusSignal.java:222)
	at org.freedesktop.dbus.connections.AbstractConnection$3.run(AbstractConnection.java:772)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.IllegalArgumentException: java.lang.ClassCastException@1858f7
	at sun.reflect.GeneratedConstructorAccessor22.newInstance(Unknown Source)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at org.freedesktop.dbus.messages.DBusSignal.createReal(DBusSignal.java:215)
	... 4 common frames omitted

What seems to happen is that while getting the signal constructor parameters in Message.getParameters(), the array of object paths is interpreted as one object path (the one path value is all the object path strings mangled together plus some garbage inbetween). Then when giving the parameters to Reflection, an exception is thrown as the wrongly deserialized constructor param is ObjectPath and not List<ObjectPath>.

2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - extract(a(oa{sv})ao,#1194, {0,0}
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - Extracting type: a from offset 0
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - aligning to a
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - Reading array of size: 0
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - aligning to (
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - Aligned type: (oa{sv})ao 8 9
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - Extracted: [] (now at 8)
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - Extracting type: o from offset 8
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - aligning to o
2018-07-19 15:38:20 [DBus Worker Thread-1] TRACE o.f.dbus.messages.DBusSignal - Extracted: I/net/connman/service/wifi_80d21d5f4569_42656c6b696e2e35333945_managed_psk>/net/connman/service/wifi_80d21d5f4569_4755455354_managed_noneU/net/connman/service/wifi_80d21d5f4569_416c6578616e64657273206950686f6e65_managed_pskQ/net/connman/service/wifi_80d21d5f4569_434552545f55505f54455354_managed_ieee8021xQ/net/connman/service/wifi_80d21d5f4569_4c414e2d4e41432d54455354_managed_ieee8021xA/net/connman/service/wifi_80d21d5f4569_4853532d444556_managed_pskE/net/connman/service/wifi_80d21d5f4569_4b6f6e666572656e7a_managed_pskA/net/connman/service/wifi_80d21d5f4569_4553583547487a_managed_psk=/net/connman/service/wifi_80d21d5f4569_4553455854_managed_pskC/net/connman/service/wifi_80d21d5f4569_4553494e54_managed_ieee8021x?/net/connman/service/wifi_80d21d5f4569_776c6e6c6368_managed_psk?/net/connman/service/wifi_80d21d5f4569_hidden_managed_ieee8021x?/net/connman/service/wifi_80d21d5f4569_4c414e_managed_ieee8021x9/net/connman/service/wifi_80d21d5f4569_455358_managed_psk9/net/connman/service/wifi_80d21d5f4569_52494f_managed_pskE/net/connman/service/wifi_80d21d5f4569_434755455354_managed_ieee8021x (now at 1195)

Specifically, after having parsed the empty first array, the second round of extractOne method for the non-empty object path array is given an offset array which seems to have its first element (signature offset) one element too far and this makes the method try to parse an array of object paths as one. Hence the first round of extractOne of the two seems to update the offset to a value one too many. When I debug and set the signature offset to a value one less for extractOne after extracting the empty array, the parameters are deserialized correctly.

This is happening in (little-endian) signals with this exact signature, in 2.7.* and 3.0 both.

[hypfvieh]

Would you please share some parts of your code, so I can try to reproduce this issue myself?

[haggish]

Please see the commited unit test in my fork, if you debug it and change the offset array input for second extractOne call from [10, 8] to [9, 8], it will serialize the example message body properly.

[hypfvieh]

I also want to try it against connman.
All my tries right now resulted in a different error than yours, so please show me your implementation of ServicesChangeHandler.

[haggish]

When I am monitoring Connman DBus traffic with gdbus monitor --system --dest net.connman:

/: net.connman.Manager.ServicesChanged (@a(oa{sv}) [], [objectpath '/net/connman/service/wifi_80d21d5f4569_52494f_managed_psk', '/net/connman/service/wifi_80d21d5f4569_4755455354_managed_none'])

(seems legit?)

At approximately same time I hit a breakpoint that deserializes the parameters to

result = {Object[2]@5358} 
 0 = {Vector@5359}  size = 0
 1 = {ObjectPath@5360} "9   /net/connman/service/wifi_80d21d5f4569_52494f_managed_psk   >   /net/connman/service/wifi_80d21d5f4569_4755455354_managed_none "

The handler (inner class):

 private class ServicesChangeHandler implements DBusSigHandler<IDBusConnmanManager.ServicesChanged> {
        @Override
        public void handle(IDBusConnmanManager.ServicesChanged s) {
            // ...
        }
    }

Signal (inner class), this is being constructed when getting the CCE:

class ServicesChanged extends DBusSignal {
		public final String path;
		public final List<PathProperties> changedServices;
		public final List<Path> removedServices;

		public ServicesChanged(String path, List<PathProperties> changedServices, List<Path> removedServices)
				throws DBusException {
			super(path, changedServices, removedServices);
			this.path = path;
			this.changedServices = changedServices;
			this.removedServices = removedServices;
		}
	}

[hypfvieh]

This is really a tough one.
Changing the extractone method to fix your issue, will then break the usage of structs (TestAll Unit test will fail).

I'll take a deeper look into it. Maybe the whole parsing stuff should be re-designed.

[hypfvieh]

So I think I got the solution.

After digging around in the Message and Marshalling class, I think there is nothing wrong.

The mistake in my view is in your PathProperties class (the part of your code, you did not publish).
PathProperties have to extend Struct and has to use the Position annotation.

The provided unit test fails, because using Message.extract directly behaves different then using the Marshalling.deSerializeParameters.

If you need custom classes like in this case (a holder which extends Struct), Message.extract will not work, as it does not know about your custom class.
You need to use Marshalling.deSerializeParameters and provide the required parameters.

I updated the unit test to do so and also implemented a proper 'Struct', so unit test is now fine.
Also my short tests with connman in a virtual machine are working like expected.

For completeness, here is my test code I used to talk to connman:

import java.util.List;
import java.util.Map;

import org.freedesktop.dbus.DBusPath;
import org.freedesktop.dbus.ObjectPath;
import org.freedesktop.dbus.Struct;
import org.freedesktop.dbus.annotations.DBusInterfaceName;
import org.freedesktop.dbus.annotations.Position;
import org.freedesktop.dbus.connections.impl.DBusConnection;
import org.freedesktop.dbus.connections.impl.DBusConnection.DBusBusType;
import org.freedesktop.dbus.exceptions.DBusException;
import org.freedesktop.dbus.handlers.AbstractSignalHandlerBase;
import org.freedesktop.dbus.interfaces.DBusInterface;
import org.freedesktop.dbus.messages.DBusSignal;
import org.freedesktop.dbus.types.Variant;

public class TestConnman {

    public static void main(String[] args) throws DBusException, InterruptedException {
        DBusConnection connection = DBusConnection.getConnection(DBusBusType.SYSTEM);
        connection.addSigHandler(IServicesChanged.ServicesChanged.class, new ServiceChangedHandler() {

            @Override
            public void handle(IServicesChanged.ServicesChanged _signal) {
                System.out.print("Received signal.");
                System.out.println("Removed devices: " + _signal.getRemoved());
                System.out.println("Changed properties: " + someDataToString(_signal.getChanged()));
            }

            private String someDataToString(List<SomeData> changed) {
                StringBuilder sb = new StringBuilder();
                for (SomeData string : changed) {
                    sb.append("DbusPath: ").append(string.getObjectPath()).append("   -   Properties: ")
                            .append(string.getProperties()).append("\n");
                }
                return sb.toString();
            }
        });

        Thread.sleep(100000);

    }

    public abstract static class ServiceChangedHandler
            extends AbstractSignalHandlerBase<IServicesChanged.ServicesChanged> {

        @Override
        public Class<IServicesChanged.ServicesChanged> getImplementationClass() {
            return IServicesChanged.ServicesChanged.class;
        }

    }

    @DBusInterfaceName("net.connman.Manager")
    interface IServicesChanged extends DBusInterface {

        public static class ServicesChanged extends DBusSignal {

            public final String           objectPath;

            public final List<SomeData>   changed;
            public final List<ObjectPath> removed;

            public ServicesChanged(String _objectPath, List<SomeData> _k, List<ObjectPath> _removedItems)
                    throws DBusException {
                super(_objectPath, _k, _removedItems);
                objectPath = _objectPath;

                changed = _k;
                removed = _removedItems;
            }

            public String getObjectPath() {
                return objectPath;
            }

            public List<SomeData> getChanged() {
                return changed;
            }

            public List<ObjectPath> getRemoved() {
                return removed;
            }

        }
    }

    public static class SomeData extends Struct {
        @Position(0)
        public DBusPath                objectPath;
        @Position(1)
        public Map<String, Variant<?>> properties;

        public SomeData(DBusPath objectPath, Map<String, Variant<?>> properties) {
            this.objectPath = objectPath;
            this.properties = properties;
        }

        DBusPath getObjectPath() {
            return objectPath;
        }

        void setObjectPath(DBusPath _objectPath) {
            objectPath = _objectPath;
        }

        Map<String, Variant<?>> getProperties() {
            return properties;
        }

        void setProperties(Map<String, Variant<?>> _properties) {
            properties = _properties;
        }

    }

}

[littlefreaky]

I had the exact same problem. The issue seems to be that, if the changedServices list is empty, the optimizePrimitives method in the Message class wants to skip the signature of the struct (Message:1054). The length of the struct signature is calculated by using Marshalling.getJavaType, which returns a size that is one off for structs if I read it correctly. See my small fix above.