Bug 8365

Created attachment 7094 [details] [review]
proposed patch

This isn't right, because the "invalid parameters" return values are not in 
the API contract.

Say I have this function:

int
frobate(char *a, int b)
{
   return_val_if_fail(a != NULL, NULL);
   return_val_if_fail(b >= 1, NULL);

   return strdup_printf("%s: %d", a, 42 + b);
}

the docs are:
 - it returns the string "a: 42+b"
 - it returns NULL if there is not enough memory to allocate
   the returned string
 - a must not be NULL or the results are undefined
 - b must be >= 1 or the results are undefined

There are two _likely_ outcomes if you pass a=NULL or b<1, which are a zero 
return, or (with --disable-checks) the function just tries to use the invalid 
parameters, which means you could get a string like "(null): 42+b" on Linux, 
or a crash on Solaris where null for %s is not allowed.

Now say the function is:

int
frobate(char *a, int b)
{
   if (a == NULL)
      return NULL;
   else
      return strdup_printf("%s: %d", a, 42 + b);
}

Now the docs are different, because a==NULL is a VALID parameter handled in a 
defined way. Now the docs are that if a is NULL, the function returns NULL. 
Note that in this case, OOM cannot be reliably detected.

return_if_fail() is just the same, conceptually, as assert(). It's a "friendly 
assert" that doesn't actually abort(), but it's still an assertion.

My point is that the programmer needs to know _exactly_ when and what is
returned. The API contract should be _told_ in the function comment (or where
are you telling it?). I can change the comment to 'results are undefined', but I
think it's even worse than saying what happens.

The cases when it crashes are not the problem (then it does not return).

So: the API contract must be told, or the programmer needs to guess -- and then
there is a risk of misunderstanding. E.g. the programmer could _count on_ the
fact that it crashes on invalid arguments (and test accordingly), but instead it
returns NULL.

Can you just think of these alternatives: A) explicitly telling in the function
comment that the results are undefined if invalid parameters are passed, or B)
not telling what happens if invalid parameters are passed.

I think alternative A would remind the programmer to validate the parameters in
case he really is relying on the result. In case of B, the programmer would
probably forget about the validity and/or trust in the _documented_ return value(s).

You can see how e.g. the man pages do pretty good work on the documentation.
They even say when the value of some pointer is undefined. The D-Bus API
documentation does nothing like that.

I think it would improve the docs to add "the interface name must be non-NULL 
and syntactically valid, or the results are undefined" or "the connection must 
not be NULL or the results are undefined"

It's actively misleading to document that there's any particular return value 
if the parameters are invalid, because for most parameters, it is not 
true. "undefined" is meant in a very literal sense here: the library may print 
a warning, crash, and/or return some junk value.

I agree almost all what you are writing, but I think we should avoid returning
'some junk value' at all costs. This is because it can only harm the user of the
library, it can never benefit the user. However, if we say that 'in the case
validation is compiled, NULL is returned for invalid parameter', then the user
could at least prepare for the case.

The alternative is that we never return on invalid parameters. I think changing
the semantics of the API when validation is or is not compiled is not good: it
would be better that return_val_if_fail() etc. would only print an error if
validation is compiled in (and later the function could crash -- but the user
would see the reason for the crash from the warning message). So, they would
never cause the function to return. That way the semantics of the function would
not change depending on compilation of checks and validations.

I would have no objection to changing to the equivalent of 
G_DEBUG=fatal_warnings for glib (i.e. it prints the warning, then aborts), at 
least by default. We might or might not add a dbus_set_fatal_warnings to let 
people go back to the less aggressive "only warn and return a possibly-safe 
but undefined value" mode.

I would say abort() instead of warn and continue, because continuing could 
have all kinds of unknown effects including buffer overflows or silently 
corrupting data.

--disable-checks is not encouraged for a typical dbus install; its purpose is 
for embedded systems worried about code size and other corner cases. Correct 
programs will not change behavior with --enable-checks vs. --disable-checks.

I agree. I also think that the Glib way of allowing non-fatal warnings (that
actually are dangerous bugs) is one big mistake that the Glib designers made. It
has caused us lot of trouble when developing the Nokia 770 (think about how to
force fixing the bugs when just making them fatal is _not_ an option due to the
internal processes).

I would strongly suggest that D-Bus only has fatal warnings (abort()) if they
are compiled in. The reason for this is that returning values in order to avoid
the crash is only helping to hide bugs in the client code -- so it's rather
dangerous than helpful.

So what is the status of this patch.  Do we add it or does it need changes? 
From the descussion it seems to need some changes.

I could make a new patch if Havoc agrees on the changes. As I said, I would
prefer that the semantics are the same, despite of what checks have been
compiled in (i.e. no new return values if checks are compiled). Failing check
could mean an optional warning followed by abort(). I would not return 'garbage
value' in case of a failing check.

abort or not doesn't affect the docs. We aren't going to support a defined 
behavior if people pass junk args. I think the abort() would be a good option 
to have (like glib's fatal warnings mode), but the docs are still "it's 
undefined" - it's not in the API contract.

If someone wants to be able to pass junk args, they can create a libdbus 
wrapper or binding that validates their args. But I don't want to maintain 
these guarantees in libdbus itself, the libdbus policy is that you must use 
the API correctly and as documented or the results are undefined.

I'm not asking for defined operation in case of invalid arguments (that's
impossible). My point is that the documented function returns should be the only
returns the user ever sees, because otherwise there is a risk of misinterpretation.

I think we could consider this fixed now that the library causes crash on
invalid arguments, because that keeps the semantics clear (no surprise returns).