Created attachment 22206 [details] [review]
A patch to message.c to add marshal() and demarshal() methods

Removed #include<stdio.h>, which was being used for debugging.

As discussed on IRC, the problem is that dbus_message_marshal() does not fill in the message's 'length' header (which is done in _dbus_message_lock) before marshalling it.

I attempted to backport the patch from #22141 to my installation of libdbus-1.2.3.  Unfortunately, this did not solve the problem.  However, it did change the behavior.  A sample python session:

>>> import dbus
>>> x = dbus.lowlevel.SignalMessage('/a/b','c.d','e')
>>> x.append('a test string')
>>> s = x.marshal()
>>> s
'l\x04\x01\x01\x12\x00\x00\x00\x00\x00\x00\x007\x00\x00\x00\x01\x01o\x00\x04\x00\x00\x00/a/b\x00\x00\x00\x00\x02\x01s\x00\x03\x00\x00\x00c.d\x00\x00\x00\x00\x00\x03\x01s\x00\x01\x00\x00\x00e\x00\x00\x00\x00\x00\x00\x00\x08\x01g\x00\x01s\x00\x00\r\x00\x00\x00a test string\x00'
>>> y = dbus.lowlevel.Message.demarshal(s)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: Message is corrupted

The 4th byte, which was previously \x00, is now \x12.  This seems to reflect the length of the contents, which was added by the call to dbus_message_lock().

Unfortunately, it appears that more problems still remain in the marshalling or demarshalling code.

(In reply to comment #3)
> Unfortunately, it appears that more problems still remain in the marshalling or
> demarshalling code.

I see the same results when I do the same kind of thing with raw libdbus. :(

A-ha!

s[8:12] are the message's serial number, and are all zeroes. dbus_message_demarshal doesn't accept messages that have a zero serial number.

The spec never actually says that serials need to be non-zero, but given that this implies that the daemon will throw away messages unless the serial is non-zero, I propose amending the specification to match the implementation. I guess you'll need to wrap dbus_message_set_serial () in the Python binding, which makes me kind of sad.

(In reply to comment #5)
> I
> guess you'll need to wrap dbus_message_set_serial () in the Python binding,
> which makes me kind of sad.

I should explain my sadness: dbus_message_set_serial () will assert if you call it twice, so you'll probably have to wrap it to check if the message has a serial, and if it does throw a Python assertion rather than calling it again.

(In reply to comment #5)
> A-ha!
> 
> s[8:12] are the message's serial number, and are all zeroes.
> dbus_message_demarshal doesn't accept messages that have a zero serial number.
> 
> The spec never actually says that serials need to be non-zero, but given that
> this implies that the daemon will throw away messages unless the serial is
> non-zero, I propose amending the specification to match the implementation.

I'm not convinced.

You see, the reason I (personally) want to marshal and demarshal messages is to serialize Python D-Bus objects to disk (efficiently), and later reconstitute them back into D-Bus objects.  The marshaled messages never have to go through the daemon, so I don't mind that the daemon wouldn't accept them.  Therefore, I think a better solution would be to make dbus_message_demarshal accept messages with a zero serial number, since serial number is perfectly irrelevant to me.

An even better solution might be to do this and also add python bindings for set_serial, so that users can use dbus_message_demarshal without setting a serial number, but (hypothetical) users who _do_ need to set a serial number from python can.  However, as I'm the only person I know of who's requested this feature, I doubt a python binding for set_serial would get used.

(In reply to comment #7)
> You see, the reason I (personally) want to marshal and demarshal messages is to
> serialize Python D-Bus objects to disk (efficiently), and later reconstitute
> them back into D-Bus objects.  The marshaled messages never have to go through
> the daemon, so I don't mind that the daemon wouldn't accept them.  Therefore, I
> think a better solution would be to make dbus_message_demarshal accept messages
> with a zero serial number, since serial number is perfectly irrelevant to me.

I'd like it to be like this: the message is syntactically well-formed, even if the daemon would never have accepted it.

However, since people are frequently (and correctly?) reluctant to update their libdbus, I think in the short-to-medium term, you'll also need to set a stub serial number when doing your serialization.

> An even better solution might be to do this and also add python bindings for
> set_serial, so that users can use dbus_message_demarshal without setting a
> serial number, but (hypothetical) users who _do_ need to set a serial number
> from python can.  However, as I'm the only person I know of who's requested
> this feature, I doubt a python binding for set_serial would get used.

Since set_serial is a "call no more than once" thing, I think I'd rather bind it as an optional kwarg to the constructor:

    m = dbus.lowlevel.SignalMessage(..., serial=1)
    m.append(*payload, signature='...')
    f.write(m.marshal())

If it's exposed as a method, it must raise a suitable exception (I'd use either ValueError or a DBusException subtype) rather than letting libdbus warn (which is fatal on approximately everything except Debian).

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus-python/issues/6.