You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2004/11/09 18:37:46 UTC
[PATCH] abort or assert?
This patch converts aborts into asserts where it seems most appropriate.
- Julian
Re: [PATCH] abort or assert?
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
[...]
> I think I've made my point.
I disagree with many of the points you are making, but will not argue further
at the moment. I will wait for others to give their opinions.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] abort or assert?
Posted by Branko Čibej <br...@xbc.nu>.
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
Re: [PATCH] abort or assert?
Posted by Julian Foad <ju...@btopenworld.com>.
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). For the present
purpose, I regard as a separate issue the question of whether asserts should be
enabled even in release builds.
I conclude that all of the changes in this patch are appropriate, modulo the
question of whether asserts should be enabled even in release builds. This
covers about three quarters of the abort() calls in our code; the others are
either best left alone or more difficult to evaluate.
- Julian
> Index: subversion/libsvn_fs_base/reps-strings.c
> ===================================================================
> --- subversion/libsvn_fs_base/reps-strings.c (revision 11796)
> +++ subversion/libsvn_fs_base/reps-strings.c (working copy)
> @@ -421,12 +421,13 @@
>
> /* Read in our REP. */
> SVN_ERR (svn_fs_bdb__read_rep (&rep, fs, rep_key, trail));
> + assert (rep->kind == rep_kind_delta || rep->kind == rep_kind_fulltext);
Only these two values are defined for the "kind" field, so any other value is
due to a bug.
The same change is made throughout this file.
> Index: subversion/libsvn_fs_base/trail.c
> ===================================================================
> --- 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);
This is already documented right there as a bug detection.
> 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.
> @@ -443,8 +443,7 @@
> {
> /* 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);
Documented right there.
(And the same again in this file.)
> Index: subversion/libsvn_fs_base/util/fs_skels.c
> ===================================================================
> --- subversion/libsvn_fs_base/util/fs_skels.c (revision 11796)
> +++ subversion/libsvn_fs_base/util/fs_skels.c (working copy)
> @@ -961,6 +962,7 @@
>
> /** Do the kind-specific stuff. **/
>
> + assert (rep->kind == rep_kind_delta || rep->kind == rep_kind_fulltext);
Same as in subversion/libsvn_fs_base/reps-strings.c, above.
> Index: subversion/libsvn_diff/token.c
> ===================================================================
> --- 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);
Argument validation.
> 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.
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 11796)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -946,8 +946,7 @@
> svn_wc_status_t *parent_status;
>
> /* Don't do this. Just do NOT do this to me. */
> - if (pb && (! path))
> - abort();
> + assert (!pb || path);
Argument validation.
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 11796)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -269,8 +269,7 @@
> struct bump_dir_info *bdi;
>
> /* Don't do this. Just do NOT do this to me. */
> - if (pb && (! path))
> - abort();
> + assert (!pb || path);
Argument validation.
> @@ -547,8 +546,7 @@
> struct file_baton *f = apr_pcalloc (pool, sizeof (*f));
>
> /* I rather need this information, yes. */
> - if (! path)
> - abort();
> + assert (path);
Argument validation.
> @@ -1009,9 +1007,7 @@
>
> /* Semantic check. Either both "copyfrom" args are valid, or they're
> NULL and SVN_INVALID_REVNUM. A mixture is illegal semantics. */
> - if ((copyfrom_path && (! SVN_IS_VALID_REVNUM (copyfrom_revision)))
> - || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM (copyfrom_revision))))
> - abort();
> + assert ((copyfrom_path != NULL) == SVN_IS_VALID_REVNUM (copyfrom_revision));
Argument validation.
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 11796)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -1162,8 +1162,7 @@
> target = parent_dir;
> while (strcmp (target, base_dir))
> {
> - if (target[0] == '/' && target[1] == '\0')
> - abort();
> + assert (! (target[0] == '/' && target[1] == '\0'));
Checking that the "base_dir" which was calculated to be a parent of all targets
is actually a parent of the present target. Thus, a bug check.
> Index: subversion/tests/libsvn_fs_base/skel-test.c
> ===================================================================
> --- subversion/tests/libsvn_fs_base/skel-test.c (revision 11796)
> +++ subversion/tests/libsvn_fs_base/skel-test.c (working copy)
> @@ -172,12 +173,9 @@
> static void
> put_implicit_length_byte (svn_stringbuf_t *str, char byte, char term)
> {
> - if (! skel_is_name (byte))
> - abort ();
> - if (term != '\0'
> - && ! skel_is_space (term)
> - && ! skel_is_paren (term))
> - abort ();
> + assert (skel_is_name (byte));
> + assert (term == '\0' || skel_is_space (term) || skel_is_paren (term));
> +
Argument validation.
Same throughout this file.
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 11796)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -252,8 +254,7 @@
> const char *full_path;
>
> /* A path relative to nothing? I don't think so. */
> - if (path && (! pb))
> - abort();
> + assert (!path || pb);
Argument validation.
> Index: subversion/libsvn_fs_fs/dag.c
> ===================================================================
> --- subversion/libsvn_fs_fs/dag.c (revision 11796)
> +++ subversion/libsvn_fs_fs/dag.c (working copy)
> @@ -628,10 +628,7 @@
> /* Oh, give me a clone...
> (If they're the same, we haven't cloned the transaction's root
> directory yet.) */
> - if (svn_fs_fs__id_eq (root_id, base_root_id))
> - {
> - abort ();
> - }
> + assert (! svn_fs_fs__id_eq (root_id, base_root_id));
Though I am not certain, this looks like it is detecting an error in the usage
of the function.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] abort or assert?
Posted by Branko Čibej <br...@xbc.nu>.
Julian Foad wrote:
> This patch converts aborts into asserts where it seems most appropriate.
I think you'll have to define what "appropriate" means in this context
before we dare commit any changes. Interestingly enough, HACKING says
nothing about assert vs. abort, so our usage patterns mostly grew from
general consensus and code reviews. Thnking back on this, I find the
situation a bit frightening because, as I said in earlier posts,
changing an abort to an assert could mean the difference between a crash
and data corruption.
I'd suggest we use assert in favour of abort only when we can prove that
disabling the assert can't result wrong behaviour -- i.e., when the
assert is truly a debugging aid used to find a bug earlier, but the
presence of the assert doesn't affect an invariant.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org