On Tue, 2008-06-17 at 10:53 +0200, Vincent Lefevre wrote:
> On 2008-06-16 16:17:05 +0100, Julian Foad wrote:
> > Do you mean in the case of corruption caused by a malicious user? Yes,
> > you might well be right, but that condition is impossible to distinguish
> > programmatically so it's a moot point.
>
> Of course, but what I want to say is that in the doubt, aborting
> immediately is probably safer (because less code is run), whereas
> in the other cases, if an error is just reported, can anything
> really useful be done without the risk to corrupt data? I mean,
> as the failed assertion can imply incorrect data in some other
> variables or memory structures, trying to fix things and ignore
> the error (or trying to do some clean-up before terminating with
> a "more useful" error for the user) can corrupt data even more.
> So, I doubt this is useful enough compared to the risk of losing
> data, even if this risk is quite low... unless you know reasons
> for which there will be no problems.
I think assertions are most often triggered by simple bugs that have no
risk of writing corrupt data to the repository or to anywhere, where the
code merely encounters some unexpected combination of inputs that are
individually valid. In these common cases, there is no danger in
allowing the program to continue (either just to do a bit of cleaning up
before exiting, or to keep going with other operations).
Remember that the conditions checked by assertion statements are not
very different from, and sometimes exactly the same as, conditions that
we already check with normal validation checks:
/** ... and @a rev should be of kind "head" or "base", otherwise
* we'll throw an error. */
svn_func1(svn_opt_revision_t rev)
{
/* This API is designed such that it's our responsibility
to validate this argument. */
if (rev->kind != svn_opt_revision_head
&& rev->kind != svn_opt_revision_base)
return error(SVN_ERR_INVALID_REVISION_KIND);
...
}
/** ... and @a rev must be of kind "head" or "base". */
svn_func2(svn_opt_revision_t rev)
{
/* This API is designed with API requirements that the
* caller must obey. To assist development and debugging
* we enforce the requirements with an assertion. */
SVN_ERR_ASSERT(rev->kind == svn_opt_revision_head
|| rev->kind == svn_opt_revision_base);
...
}
So it doesn't seem realistic to expect that assertions will catch cases
of potential data corruption much more often than any other checks will.
Therefore, aborting the program as soon as any assertion fails would not
be a good compromise between usability and security.
> > I think your concern is perhaps that higher-level code might ignore
> > assertions unintentionally or without due consideration. The answer here
> > is to help higher level code to do the right thing. For instance, we
> > could make the standard error-ignoring mechanisms like
> > "svn_error_clear(svn_error_t *err)" NOT ignore assertions, and provide a
> > separate "svn_assertion_clear(svn_error_t *err) for those few cases
> > where code needs to handle assertions in a special way.
> >
> > Does that approach address your concerns?
>
> Well, without knowing what higher-level code does exactly, it is
> difficult to say. Also can it make the difference between a failed
> assertion (that indicates some bug) and a simple error coming from
> the environment (e.g. something like a file not found in a low-level
> function).
Yes it can tell the difference: I meant that assertions would have a
special error code (or codes) so that things like "svn_error_clear" can
tell whether a particular error is an assertion or not. Like this
(pseudo-code):
#define SVN_ERR_ASSERTION 12345
#define SVN_ERR_ASSERT(expression) \
if (!expression) return svn_error_create(SVN_ERR_ASSERTION);
#define svn_error_clear(err) \
if (err->apr_err == SVN_ERR_ASSERTION) /* abort or something */
else /* really clear the error */
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-17 12:31:05 CEST