[pulseaudio-discuss] asserts active by default

[Lennart Poettering]

On Tue, 02.09.08 12:04, Colin Guthrie (gmane at colin.guthr.ie) wrote:

> > What's the common practice for asserts in the OSS world in this kind
> > of libraries?
> > 
> > I've seen in other projects a "--disable-assert" argument to the
> > ./configure script. Would that (or --enable-assert) be an option to
> > consider?
> 
> The asserts indicate when an invalid use of the API is encountered (or a 
> bug of course!). When an assert happens it indicates an error condition 
> and in pulse, essentially protect against buffer overflows and other 
> such conditions.

The PulseAudio client libraries check most API misuses via non-fatal
checks (i.e. via PA_CHECK_VALIDITY_RETURN_NULL or a similar
macro). The only places where we do assert checks are in internal APIs
where it is obvious that we are encountering a programming bug inside
of PA -- or when things are so broken we cannot or should not go
on. Primary example for this is if context pointers are NULL or their
ref counters are bogus.

Having said that it happens from time that an assert is inappropriately
placed: i.e. it should be a non-fatal check but became a fatal assert
instead. If that happens, it is a bug and should be fixed.

Different systems handle errors differently. glib as one example likes
to tape even over the most blatant programming errors by catching
almost everything non-fatally. I consider that a bad idea,so I decided
to not do it. glibc on the other hand never does checking of input
parameters at all, presumably due to perfomance reasons. I consider
this other extreme a bad idea too. So I find my way in the middle. Do
input checking with assert where appropriate, check non-fatally
everywhere else.

> If the asserts were disabled, then a significant rework in terms of 
> error handling would need to be performed. If a catchable error occurs 
> in pulse, an assert is not used.

This is actually not true. PA has two kinds of asserts: pa_assert()
and pa_assert_se(). The normal pa_assert() can safely be optimized
completely away and is so if NDEBUG is defined. OTOH pa_assert_se() is
always performed, regardless of the setting of NDEBUG. Directly
security relevant checks or checks with side-effects are always
enclosed in pa_assert_se() and not in pa_assert(). ('se' stands for
'side effect').

Of course, it might happen from time to time that a pa_assert() is
placed where a pa_assert_se() belongs, which would then be a bug and
needs fixing.

Haveing said all this: even though it should be safe to
define NDEBUG without a "significant rework in terms of error
handling", I cannot really recommend that. (see my earlier mail
for an explanation)

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4