Re: [Qemu-devel] [RFC PATCH] osdep.h: Prohibit disabling assert() in supported builds

[Peter Maydell]

On 11 September 2017 at 11:30, Paolo Bonzini <address@hidden> wrote:
> On 06/09/2017 07:26, Thomas Huth wrote:
>> You asked for opinions, so here's mine: I agree with you, please do
>> *not* add a new QEMU-specific construct here. assert() should be a
>> well-known C construct that every programmer should have understood. You
>> also need it for other projects. If you haven't understood that it's a
>> macro and has side-effects, you should learn it (e.g. during patch
>> review), not avoid it, otherwise you'll run into problems in another
>> project that is using it again.
>>
>> But IMHO we should still try to get rid of wrong usage of assert() in
>> the QEMU sources. So maybe we could allow building with NDEBUG one day
>> for the brave people who need the extra percent of additional speed.
>
> It's not only about the side effects.  There are a couple cases in
> migration (IIRC) where our error propagation is not up to the task, and
> failing assertions are used to validate untrusted input.  NDEBUG in that
> case can introduce a known vulnerability.
>
>> But as long as we're not there, I think this patch is a good thing to
>> avoid wrong expectations.
>
> Agreed.

Mmm. The only thing that makes me hesitate is that this is moving
the flagging up that we rely on assert() causing a failure away
from the places where we do that -- if we want in future to fix
these migration issues then they'll get harder to find.
(If we already think we have more of those that aren't marked
by testing NDEBUG then we'd already need to do a wider code audit
anyway, though...)

thanks
-- PMM