kfogel@collab.net wrote:
> 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.
I really really don't like the idea of (3).
I think the idiom it destroys is useful and elegant.
(2) is messy, and (4) is not so good, because error cases are much less
exercised than success cases.
Therefore, my vote is for (1). Obviously, that means I'm volunteering to do
some of the work.
Let code review and committer vigilance be our protection against
re-occurrence of this class of bug.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Dec 16 16:20:12 2004