You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2004/12/16 00:08:44 UTC
Questions about svn_error_clear().
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
Re: Questions about svn_error_clear().
Posted by Philip Martin <ph...@codematters.co.uk>.
"Max Bowsher" <ma...@ukf.net> writes:
> kfogel@collab.net wrote:
>>
>> 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 don't like it much either.
> 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.
While (4) is not a solution I'd like to see it as part of
SVN_DEBUG_ERROR.
> Therefore, my vote is for (1). Obviously, that means I'm volunteering
> to do some of the work.
We have evidence that the bug occurred once, but it may not occur
anywhere else and I'd be surprised if it is widespread. I don't think
any such review is urgent (and the code paths covered by the testsuite
can mostly be checked by the combination of pool-debugging and
valgrind as the error will often be detected as a freed memory read).
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by kf...@collab.net.
Justin Erenkrantz <ju...@erenkrantz.com> writes:
> Not agreed on #4: it's my preference. However, I have a subtle
> variant on that: could we add a level of indirection to the allocated
> svn_error_t's to make it known when it has been cleared? It's
> slightly different than #4, but would ensure we won't segfault if we
> called repeatedly.
Well, svn_error_t is an open type. I'm not sure how to add a level of
indirection compatibly.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, December 16, 2004 3:17 PM +0000 Max Bowsher <ma...@ukf.net>
wrote:
>> 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.
I agree: 3 is awful.
> (2) is messy, and (4) is not so good, because error cases are much less
> exercised than success cases.
Agreed on #2.
Not agreed on #4: it's my preference. However, I have a subtle variant on
that: could we add a level of indirection to the allocated svn_error_t's to
make it known when it has been cleared? It's slightly different than #4, but
would ensure we won't segfault if we called repeatedly.
> 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.
I don't like relying upon humans either. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by Max Bowsher <ma...@ukf.net>.
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
Re: Questions about svn_error_clear().
Posted by Branko Čibej <br...@xbc.nu>.
Garrett Rooney wrote:
> Steven Kirk wrote:
>
>>
>>>> Another option
>>>>
>>>> #define svn_error_clear(err) svn_error_clear2(&(err))
>>>>
>>>> And at the end of svn_error_clear_internal:
>>>>
>>>> *err = NULL;
>>>>
>>>> svn_error_clear() would remain in the svn libs as to not break
>>>> binary compatibility.
>>>
>>>
>>>
>>> Can't do that because it changes the signature of the function
>>> consumers of the API get when they write svn_error_clear, suddenly
>>> all their code breaks because they're not passing a pointer to a
>>> pointer to an error object. It would preserve binary compatability,
>>> but not source compatability, we need both.
>>
>>
>>
>> Sorry, I don't understand... Binaries would remain linked to
>> svn_error_clear, whilst the svn_error_clear() macro would
>> automatically redirect newly compiled code to the new
>> svn_error_clear2() function. AFAICS, you retain binary and source
>> compatibility. But then I always have had trouble understanding
>> compatability rules!
>
>
> Actually, I think I was mistaken. I wasn't fully awake when reading
> that email... It will work in the majority of cases, although I'm not
> sure if it's valid to take the address of a temporary (i.e.
> svn_error_clear(svn_function_that_returns_error()) might be problematic).
It is problematic, i.e., it can't be done.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
Steven Kirk wrote:
>
>>> Another option
>>>
>>> #define svn_error_clear(err) svn_error_clear2(&(err))
>>>
>>> And at the end of svn_error_clear_internal:
>>>
>>> *err = NULL;
>>>
>>> svn_error_clear() would remain in the svn libs as to not break binary
>>> compatibility.
>>
>>
>> Can't do that because it changes the signature of the function
>> consumers of the API get when they write svn_error_clear, suddenly all
>> their code breaks because they're not passing a pointer to a pointer
>> to an error object. It would preserve binary compatability, but not
>> source compatability, we need both.
>
>
> Sorry, I don't understand... Binaries would remain linked to
> svn_error_clear, whilst the svn_error_clear() macro would automatically
> redirect newly compiled code to the new svn_error_clear2() function.
> AFAICS, you retain binary and source compatibility. But then I always
> have had trouble understanding compatability rules!
Actually, I think I was mistaken. I wasn't fully awake when reading
that email... It will work in the majority of cases, although I'm not
sure if it's valid to take the address of a temporary (i.e.
svn_error_clear(svn_function_that_returns_error()) might be problematic).
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by Steven Kirk <st...@status-scientific.com>.
>> Another option
>>
>> #define svn_error_clear(err) svn_error_clear2(&(err))
>>
>> And at the end of svn_error_clear_internal:
>>
>> *err = NULL;
>>
>> svn_error_clear() would remain in the svn libs as to not break binary
>> compatibility.
>
> Can't do that because it changes the signature of the function
> consumers of the API get when they write svn_error_clear, suddenly all
> their code breaks because they're not passing a pointer to a pointer
> to an error object. It would preserve binary compatability, but not
> source compatability, we need both.
Sorry, I don't understand... Binaries would remain linked to
svn_error_clear, whilst the svn_error_clear() macro would automatically
redirect newly compiled code to the new svn_error_clear2() function.
AFAICS, you retain binary and source compatibility. But then I always
have had trouble understanding compatability rules!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
Steven Kirk wrote:
> Another option
>
> #define svn_error_clear(err) svn_error_clear2(&(err))
>
> And at the end of svn_error_clear_internal:
>
> *err = NULL;
>
> svn_error_clear() would remain in the svn libs as to not break binary
> compatibility.
Can't do that because it changes the signature of the function consumers
of the API get when they write svn_error_clear, suddenly all their code
breaks because they're not passing a pointer to a pointer to an error
object. It would preserve binary compatability, but not source
compatability, we need both.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by Steven Kirk <st...@status-scientific.com>.
> 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.
>
>
Another option
#define svn_error_clear(err) svn_error_clear2(&(err))
And at the end of svn_error_clear_internal:
*err = NULL;
svn_error_clear() would remain in the svn libs as to not break binary compatibility.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> > 3. Make svn_error_clear() a macro like this:
>
> Still yech, in my opinion. And it would violate our API compatibility
> rules, unless we gave it a new name.
We could give it a new name, that would be easy. Have to change a lot
of call sites, but no big deal.
> In my opinion, code at the end of a function to "clean up the error we
> might have encountered at some undefine point earlier on" is inelegant
> and error-prone with or without safeties like this. It's not always
> 100% clear how to avoid that sort of construct without duplicating a lot
> of code. But I don't think we should be making project-wide allowances
> for the sake of the few places we have that construct.
So your solution is to avoid code like that? I'm sure we can catch it
in review from now on if we want. But there are a lot of existing
places to clean up, and in some of them I'm not sure I agree with the
"inelegant" assessment. There's a reason C has 'goto'. (On the other
hand, the presence of the bug we just fixed is a good argument against
the construct.)
I think your solution boils down to (1): Audit all current uses, try
to be smarter in the future.
> > 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.
>
> That's fine with me.
I'm not sure the benefit is worth the effort; we'd still want to do
(1) anyway.
> > 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.
>
> All of the errors in a chain live in the same pool.
Thank you, that makes it click for me.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Questions about svn_error_clear().
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2004-12-15 at 19:08, kfogel@collab.net wrote:
> 2. Make it project policy to always put 'err = NULL' after
> svn_error_clear(err), and do that everywhere right now.
Yech. This would be like setting pointers to NULL every time you free
them, even if you never use the variable again, in a project which
doesn't use pools.
> 3. Make svn_error_clear() a macro like this:
Still yech, in my opinion. And it would violate our API compatibility
rules, unless we gave it a new name.
In my opinion, code at the end of a function to "clean up the error we
might have encountered at some undefine point earlier on" is inelegant
and error-prone with or without safeties like this. It's not always
100% clear how to avoid that sort of construct without duplicating a lot
of code. But I don't think we should be making project-wide allowances
for the sake of the few places we have that construct.
> 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.
That's fine with me.
> 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.
All of the errors in a chain live in the same pool.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org