Julian Foad wrote:
> Julian Foad wrote:
>
>> This patch converts aborts into asserts where it seems most appropriate.
>
>
> Brane requests that the appropriateness be carefully considered, so I
> here consider whether these are checking for bugs in the software (an
> appropriate use) rather than input data validation (inappropriate).
Ah, sorry, that's not quite what I said. I said we should first define
what "appropriate" /means/, before we make any changes. And my tentative
definition of "appropriate" is different from yours -- which I happen to
disagree with.
> For the present purpose, I regard as a separate issue the question
> of whether asserts should be enabled even in release builds.
No, this can't be a separate question. I said,
I'd suggest we use assert in favour of abort only when we can prove
that disabling the assert can't result wrong behaviour
Clearly, if we use your definition of appropriate instead of mine, the
question about when to use assert is crucial. Sorry but I think you
jumped the gun a bit with your patch. So far the discussion hasn't
reached any conclusion and until it does, I won't support (-1, that is)
any such changes. Especially since some of them are downright dangerous,
see below.
[snip]
>> ===================================================================
>> --- subversion/libsvn_fs_base/trail.c (revision 11796)
>> +++ subversion/libsvn_fs_base/trail.c (working copy)
>> @@ -105,10 +107,9 @@
>> trail->undo = 0;
>> if (use_txn)
>> {
>> - /* If we're already inside a trail operation, abort() -- this is
>> + /* If we're already inside a trail operation, abort -- this is
>> a coding problem (and will likely hang the repository
>> anyway). */
>> - if (bfd->in_txn_trail)
>> - abort();
>> + assert (! bfd->in_txn_trail);
>
> Index: subversion/libsvn_fs_base/trail.c
>
> This is already documented right there as a bug detection.
Yes, it detects a bug that, if allowed to "proceed", will wedge the
repository /without/ any indication as to why. The abort will wedge it
too, but at least a quick look at the coredump will immediately tell you
the cause.
>
>> Index: subversion/libsvn_fs_base/util/skel.c
>> ===================================================================
>> --- subversion/libsvn_fs_base/util/skel.c (revision 11796)
>> +++ subversion/libsvn_fs_base/util/skel.c (working copy)
>> @@ -361,8 +362,7 @@
>> int length_len;
>>
>> length_len = svn_fs_base__putsize (buf, sizeof (buf),
>> skel->len);
>> - if (! length_len)
>> - abort ();
>> + assert (length_len);
>
>
> This is checking whether the value overflows its 200-byte buffer.
> Since the value is passed as an apr_size_t which cannot have more than
> 200 digits, any such failure must be due to a bug.
Yup. And if the bug actually happens and asserts are disabled...BAM!
Arbitrary code execution, here we come...
>> {
>> /* If list_skel isn't even a list, somebody's not using this
>> function properly. */
>> - if (list_skel->is_atom)
>> - abort();
>> + assert (! list_skel->is_atom);
>
> @@ -443,8 +443,7 @@
>
> Documented right there.
>
> (And the same again in this file.)
This one might be acceptable, depending on whether the code segfaults
later on if the condition is met or not. If it doesn't, there's a good
chance of heap corruption. (I've not looked.)
>> ===================================================================
>> --- subversion/libsvn_diff/token.c (revision 11796)
>> +++ subversion/libsvn_diff/token.c (working copy)
>> @@ -76,8 +78,7 @@
>> svn_diff__node_t *parent;
>> int rv;
>>
>> - if (!token)
>> - abort();
>> + assert (token);
>
> Index: subversion/libsvn_diff/token.c
>
> Argument validation.
This one is most probably appropriate, since our policy in general is to
let the code segfault instead. Again, I've not looked.
>
>> Index: subversion/libsvn_wc/entries.c
>> ===================================================================
>> --- subversion/libsvn_wc/entries.c (revision 11796)
>> +++ subversion/libsvn_wc/entries.c (working copy)
>> @@ -1026,13 +1026,11 @@
>> if (strcmp (name, SVN_WC_ENTRY_THIS_DIR))
>> {
>> /* This is NOT the "this dir" entry */
>> - if (! strcmp (name, "."))
>> - {
>> - /* By golly, if this isn't recognized as the "this dir"
>> - entry, and it looks like '.', we're just asking for an
>> - infinite recursion to happen. Abort! */
>> - abort();
>> - }
>> +
>> + /* By golly, if this isn't recognized as the "this dir"
>> + entry, and it looks like '.', we're just asking for an
>> + infinite recursion to happen. Abort! */
>> + assert (strcmp (name, ".") != 0);
>
>
> Asserting that another part of the program recognised "." as "this
> dir" - thus, bug checking.
I'm truly puzzled by this one...hm...well, the only bug we could be
checking here is a failure to convert the path '.' to canonical form.
Does anything bad happen if SVN goes into an infinite loop here? I don't
know, but aborting is probably nicer; though returning an "internal
error" thingy might be even better.
[etc. etc. snip]
I think I've made my point.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 10 04:01:01 2004