|Summary:||Documentation for dbus_type_is_fixed claims that it can crash|
|Product:||dbus||Reporter:||Stephen Gallagher <sgallagh>|
|Component:||core||Assignee:||Simon McVittie <smcv>|
|Status:||RESOLVED FIXED||QA Contact:||Rob Taylor <rob.taylor>|
|i915 platform:||i915 features:|
dbus_type_is_basic etc.: it is an error to pass in bad typecodes
Make dbus_type_is_valid into public API
Description Stephen Gallagher 2009-03-05 07:30:44 UTC
The documentation for dbus_type_is_fixed, dbus_type_is_container and dbus_type_is_basic all claim that they will crash if passed a type that is not present in dbus-protocol.h. This is not true unless the fatal_warnings_on_check_failed flag was set to true at D-BUS compile time. This should never be true on a shipped copy of D-BUS, as it is a developer debugging tool. The documentation should be updated to reflect that use of these functions is safe for packaged copies of libdbus for use with unknown values.
Comment 1 Simon McVittie 2011-01-12 09:40:20 UTC
> This is not true unless the > fatal_warnings_on_check_failed flag was set to true at D-BUS compile time. ... which it is by default (Debian and derived distributions patch that out, but e.g. Fedora doesn't). I think the correct wording to describe the implemented behaviour would be "it is an error to pass a type not mentioned in dbus-protocol.h to this function", leaving it an implementation detail whether that results in a crash or not. It's always considered to be an error if a check in libdbus fails, whether libdbus is configured to try to carry on or to abort; it's basically the same as g_return_if_fail(), g_assert_not_reached() and g_critical() in GLib. Even better would be if we had an API function, dbus_bool_t dbus_is_type (int), which would return TRUE for (e.g.) 'i' or 'a', but not for 'f' (at the time of writing, anyway); then these functions could be documented as "it is an error if dbus_is_type() would return FALSE for the same parameter".
Comment 2 Simon McVittie 2011-03-03 09:38:44 UTC
(In reply to comment #0) > The documentation should be updated to reflect that use of these functions is > safe for packaged copies of libdbus for use with unknown values. That's not the intention, so, no. (In reply to comment #1) > I think the correct wording to describe the implemented behaviour would be "it > is an error to pass a type not mentioned in dbus-protocol.h to this function", > leaving it an implementation detail whether that results in a crash or not. Fixed on my doc-typechecks-20496 branch, I'll attach the patch in a moment. I think this is good to merge for 1.4. > Even better would be if we had an API function, dbus_bool_t dbus_is_type (int), > which would return TRUE for (e.g.) 'i' or 'a', but not for 'f' Fixed on my dbus_type_is_valid-20496 branch; this adds API, so I think we should only merged it into master (for 1.5).
Comment 3 Simon McVittie 2011-03-03 09:39:31 UTC
Created attachment 44085 [details] [review] dbus_type_is_basic etc.: it is an error to pass in bad typecodes Previously, the comments said "this function will crash", but that's not strictly true (checks can be disabled or made non-fatal). Their behaviour is undefined if you do that, though. (Review for 1.4 requested.)
Comment 4 Simon McVittie 2011-03-03 09:40:30 UTC
Created attachment 44086 [details] [review] Make dbus_type_is_valid into public API This is just as useful for bindings as dbus_signature_validate, and I think it's a good design principle to say that anything checked in a _dbus_return_if_fail should be something the caller could check for themselves. (Review for master requested.)
Comment 5 Simon McVittie 2011-03-03 09:41:47 UTC
An alternative resolution would be to make these functions silently return FALSE (without a check) on invalid typecodes. This is arguably also new API, so should probably only be done in 1.5.
Comment 6 Will Thompson 2011-03-10 10:43:54 UTC
Review of attachment 44085 [details] [review]: Looks fine.
Comment 7 Will Thompson 2011-03-10 10:45:10 UTC
Review of attachment 44086 [details] [review]: Looks reasonable.
Comment 8 Simon McVittie 2011-03-10 11:16:56 UTC
Thanks, fixed in git for 1.4.8, 1.5.0 (documentation) and 1.5.0 only (new API).