[bind10-dev] Robustness in BIND 10, was whether/when to use exceptions

[JINMEI Tatuya / 神明達哉]

At Thu, 15 Oct 2009 15:50:05 +0200,
Shane Kerr <shane at isc.org> wrote:

> Multiple processes helps because a problem that affects one component
> (say dynamic updates) does not necessarily stop other components (like
> query processing). Also, restarting the dynamic update component alone
> is likely much quicker than restarting the entire system.

I agree here.

> Exceptions help (at least in my mind, although nobody else seems to
> agree) because they allow you to handle a code error at the closest
> location possible. My reasoning:

[...]

>         If we discover a code error in each of these, in many cases we
>         do not have to restart - we can dump the operation and move on.
>         Sometimes we *do* have to simply exit, but *sometimes* we should
>         be able to use an exception handler and take some evasive
>         action.

I'm not so hopeful about this part.  Let's consider the latest
security bug (a DoS) of BIND9 that triggers an assertion failure in
dynamic update processing.  To simplify it, it happened when a
malicious update request reached this part of the code called from a
routine handling update requests:

isc_result_t
dns_db_findrdataset(..., dns_rdatatype_t type, ...)
{
	...
	REQUIRE(type != dns_rdatatype_any);
	...
}

I guess what's you're hoping is that we could do something like this:

Update::handle_request()
{
	try {
		// identify the db, then
		db->findrdataset(this->request->type, ...);
		...
	} catch (InvalidDataError& e) {
		// seems like the request is bogus.  just ignore it
		// and move on.
	}
}

DB::findrdataset(..., const RRType& type)
{
	if (type == RRType::Any) {
		// bogus operation, stop here.
		throw (InvalidDataError());
	}
	...
}

My concern is that we'll often not be so sure about the reason and
seriousness of assumption mismatch in DB::findrdataset().  Even though
this may seem to be a minor error of missing validation at the caller
side, this might actually happen because 'type' is taken from a
collapsed object in memory, which may also indicate that the system is
fundamentally broken and cannot be recovered from it safely.

Since we cannot be sure, if we want to be safe we'll end up doing:

DB::findrdataset(..., const RRType& type)
{
	if (type == RRType::Any) {
		// oops, perhaps something very bad happened.
		// throw a fatal exception, which is not
		// supposed to be caught explicitly, or is supposed to
		// cause exiting in the process of exception handling.
		throw (FatalError("assertion check failed"));
	}
	...
}

Of course, this is just a possibility and we can only be sure by
actually experimenting it.  But I hope we won't know that by having an
arbitrary-code-execution bug due to our underestimation of severity
about an assumption mismatch.

---
JINMEI, Tatuya