+++ This bug was initially created as a clone of Bug #33128 +++ Attachment #42061 [details] (a patch applied in Maemo) adds an assertion that writer->type_str can't be NULL in a particular part of the marshalling code, which was probably done to shut up a static analysis tool. It's not at all obvious whether the assertion is justified. On closer inspection of the code, it seems that type_str can be NULL initially, but everything that uses it is guarded by extra initialization which ensures that it's non-NULL by copying in the message's signature. This is deferred until after dbus_message_iter_init, so that that function can't fail with OOM. I think the right way to improve clarity here would be to rearrange the initialization so it's literally never NULL, then we can get rid of ~15 useless "is it NULL?" guards. Possibilities include: - copy the signature in dbus_message_iter_init; if that fails, set an "already had OOM" flag which we check in all other accessors - defer initializing the writer until the point at which we currently copy in the signature - preallocate the DBusString for the signature as part of the DBusMessage
Created attachment 47935 [details] [review] [PATCH 1/9] _dbus_type_writer_clear: add function to invalidate a type writer Also add a macro to assert that a type-writer is still valid.
Created attachment 47936 [details] [review] [PATCH 2/9] _dbus_message_iter_check: only assert about the writer if sig_refcount > 0
Created attachment 47938 [details] [review] [PATCH 3/9] DBusMessageIter: only set up the type writer when we open the signature This removes the only place where _dbus_type_writer_add_types and friends were needed, except for inside DBusMessage (when realigning).
Created attachment 47939 [details] [review] [PATCH 4/9] _dbus_type_writer_init_types_delayed, _dbus_type_writer_add_types, _dbus_type_writer_remove_types: remove
Created attachment 47940 [details] [review] [PATCH 5/9] assert that DBusTypeWriter is valid on entry to its extern methods The only exceptions are _dbus_type_writer_clear() and the _dbus_type_writer_init() family.
Created attachment 47942 [details] [review] [PATCH 6/9] writer_recurse_variant: set type_str to a non-NULL value This will allow us to guarantee that type_str is not, in fact, NULL.
Created attachment 47944 [details] [review] [PATCH 7/9] _dbus_type_writer_init: do not allow initialization without type_str Also assert about other preconditions, for clarity.
Created attachment 47945 [details] [review] [PATCH 8/9] Assert that allegedly valid type writers' type strings are never NULL
Created attachment 47947 [details] [review] [PATCH 9/9] Stop checking for DBusTypeWriter.type_str == NULL
Applying the patches from Bug #38285 first, for better test coverage, is recommended.
Created attachment 51347 [details] [review] [1/9 v2] _dbus_type_writer_clear: add function to invalidate a type writer (More or less unchanged)
Created attachment 51348 [details] [review] [2/9 v2] _dbus_message_iter_check: only assert about the writer if sig_refcount > 0 (Rebased onto byte-ordering fixes in master)
Created attachment 51349 [details] [review] [3/9 v2] DBusMessageIter: only set up the type writer when we open the signature (Rebased onto byte-ordering fixes in master)
Created attachment 51350 [details] [review] [4/9 v2] _dbus_type_writer_init_types_delayed, _dbus_type_writer_add_types, _dbus_type_writer_remove_types: remove (Unchanged, I think)
Created attachment 51351 [details] [review] [5/9 v2] assert that DBusTypeWriter is valid on entry to its extern methods (Unchanged, I think)
Created attachment 51352 [details] [review] [6/9 v2] writer_recurse_variant: set type_str to a non-NULL value (Unchanged, I think)
Created attachment 51353 [details] [review] [7/9 v2] _dbus_type_writer_init: do not allow initialization without type_str (Unchanged, I think)
Created attachment 51354 [details] [review] [8/9 v2] Assert that allegedly valid type writers' type strings are never NULL (Unchanged, I think)
Created attachment 51355 [details] [review] [9/9 v2] Stop checking for DBusTypeWriter.type_str == NULL
The branch referenced here is based on the one from Bug #38285, for test coverage.
Rebased, still applies cleanly to master. I won't reattach the patches since they didn't really change, but here's the branch: ssh://people.freedesktop.org/~smcv/dbus.git recursive-marshal-clarity-33870
I *think* these look good, but should probably take a closer look at the typewriter's innards tomorrow.
Comment on attachment 51352 [details] [review] [6/9 v2] writer_recurse_variant: set type_str to a non-NULL value Review of attachment 51352 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-marshal-recursive.c @@ +1954,5 @@ > + /* this is set in writer_recurse_init_and_check for a variant > + * sub-iterator ... */ > + _dbus_assert (sub->type_pos_is_expectation); > + /* ... and it means that we won't actually be writing to this > + * string, so casting away its constness is OK */ This is what concerns me, for probably obvious reasons. It does seem to be *true*, though…
(In reply to comment #23) > This is what concerns me, for probably obvious reasons. It does seem to be > *true*, though… A very valid point. I think there might actually be a bug here :-/ I tried to address this by replacing type_pos_is_expectation and type_str with: /* non-NULL whenever type_pos_is_expectation would have been FALSE */ DBusString *type_str; /* non-NULL whenever type_pos_is_expectation would have been TRUE */ const DBusString *expectation; and amending the code accordingly. Unfortunately, this hits an assertion failure in writer_recurse_array(). It seems that we can hit a situation where: * writer is not an expectation * sub *is* an expectation * we copy the contained_type into sub anyway which seems pretty disastrous, but there we go. It's probably essential for some strange reason. In my defence, the bits where DBusTypeReader is used for writing/realignment, casting away the constness of its string, are far worse...
(In reply to comment #24) > It seems that we can hit a situation where: > > * writer is not an expectation > * sub *is* an expectation > * we copy the contained_type into sub anyway This happens when writer is the write-iterator for an array (say "a{sv}"). We want to copy the contained_type "{sv}" into writer->type_str, but we do so by copying it into sub->type_str... which seems rather crazy tbh, but there you go. That can be avoided by writing it into writer->type_str instead. Unfortunately, DBusMessage does its own strange things with type writers, so I've stopped trying to get this any further for the moment. I can't help thinking that this should all be much much simpler. Perhaps fixing Bug #38288 would help.
-- 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/issues/38.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.