This mail starts with a question about the best way to fix a class of
bugs. Then at the end, it questions our current implementation of
svn_error_clear(). If you want to skip the bug discussion, and jump
straight to the svn_error_clear() stuff, just search for "surprises".
Here we go:
In r12325, we fixed a simple bug like this:
svn_error_clear(err);
err = NULL; /* ### This new line is the bugfix. ### */
Usually it's not necessary to set an error to NULL after clearing it.
But in this case it was, because at the end of that function -- far
away from the above code -- we do stuff with 'err' if it's not NULL.
Oops! :-)
Subversion contains many calls to svn_error_clear(), and some of them
might be the source of similar bugs. Mike Pilato and I talked about
ways to protect against this. Here are the choices we came up with:
1. Audit all calls to svn_error_clear() right now, add 'err = NULL'
where needed, leave the rest alone.
PROS: Minimal code change.
CONS: Somewhat time-consuming, and offers no protection against
future introductions of this bug.
2. Make it project policy to always put 'err = NULL' after
svn_error_clear(err), and do that everywhere right now.
PROS: Easy to do; easy to read. Offers theoretical protection
against making this mistake in the future... if we all
follow the policy.
CONS: Large code change, though trivial to review. Plus we're
likely to forget the policy sometimes, so the protection
is not foolproof.
3. Make svn_error_clear() a macro like this:
#define svn_error_clear(svn_error_clear__err) \
do { \
svn_error__clear_internal(svn_error_clear__err); \
(svn_error_clear__err) = NULL; \
} while (0)
PROS: Solves the problem everywhere, forever. Small code change.
CONS: Code like this in mod_dav_svn/repos.c is no longer possible:
svn_error_clear(svn_fs_open_txn(&(res2->info->root.txn),
res2->info->repos->fs,
res2->info->root.txn_name,
res2->info->repos->pool));
Note that this code would get a compile error ("invalid
lvalue in assignment"), so it's not like we could end up
accidentally calling the function twice instead of once.
However, we would have to rewrite a few places to use an
external error variable, and fix the doc string of
svn_error_clear() to not recommend this idiom.
4. Mike's interesting idea: Have svn_error_clear set all the fields
in the error to zero, so that if we try anything at all with the
error later, we'll get a quick seg fault.
PROS: Detects bugs sooner than we currently do.
CONS: You can still write the bug, you're just more likely to
stimulate it this way.
Of these four, I think I favor (3), and I'll implement it if people
agree.
While investigating the feasibility of (4), I ran into some surprises
in our current implementation of svn_error_clear(). Here's the whole
function:
void
svn_error_clear (svn_error_t *err)
{
if (err)
{
#if defined(SVN_DEBUG_ERROR)
while (err->child)
err = err->child;
apr_pool_cleanup_kill (err->pool, err, err_abort);
#endif
apr_pool_destroy (err->pool);
}
}
And here's its doc string:
/** Free the memory used by @a error, as well as all ancestors and
* descendants of @a error.
*
* Unlike other Subversion objects, errors are managed explicitly; you
* MUST clear an error if you are ignoring it, or you are leaking memory.
* For convenience, @a error may be @c NULL, in which case this
* function does nothing; thus, @c svn_error_clear(svn_foo(...))
* works as an idiom to ignore errors.
*/
But it looks like if SVN_DEBUG_ERROR() is defined, then we first cdr
down to the end of the error chain, and only clear that last error.
All the ones preceding it will not be touched.
Is this deliberate? It looks like r7403 made this happen, but from
the log message, it appears that r7403 only meant to affect the
removal of the handler, not change which pool gets destroyed.
Any idea what's going on here? I realize this is only in debug mode,
but I'd still like to understand the code :-).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Dec 16 01:09:58 2004