Re: BUG_ON() vs ASSERT()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/09/16 09:35, George Dunlap wrote:
> On Tue, Sep 13, 2016 at 7:16 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 12/09/16 16:23, Jan Beulich wrote:
>>> All,
>>>
>>> in
>>> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01201.html
>>> and
>>> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01210.html
>>> Andrew basically suggests that we should switch away from using
>>> ASSERT() and over to BUG_ON() in perhaps quite broad a set of
>>> cases. And honestly I'm not convinced of this: We've been adding
>>> quite a few ASSERT()s over the last years with the aim of doing
>>> sanity checking in debug builds, without adding overhead to non-
>>> debug builds. I can certainly see possible cases where using
>>> BUG_ON() to prevent further possible damage is appropriate, but
>>> I don't think we should overdo here.
>> I am not advocating switching all ASSERT()s to BUG_ON()s.  That would be
>> silly.
>>
>> However, ASSERT()'s as a bounds check very definitely are dangerous.  If
>> there is any uncertainty about the bounds, the check must not disappear
>> in a release build.  (Better yet, code which copes cleanly with
>> insufficient bounds).
>>
>>
>> For anyone reading this email who hasn't already worked out the details
>> of XSA-186, the data corruption issue is here:
>>
>> static int hvmemul_insn_fetch(...)
>> {
>>     unsigned int insn_off = offset - hvmemul_ctxt->insn_buf_eip;
>>     ...
>>     ASSERT(insn_off + bytes <= sizeof(hvmemul_ctxt->insn_buf));
>>     memcpy(&hvmemul_ctxt->insn_buf[insn_off], p_data, bytes);
>>     ...
>>
>> It is left as an exercise to the reader to work out how to exploit this
>> on a release build of Xen, but it is hopefully obvious that the ASSERT()
>> isn't helpful.  A BUG_ON() in this case would have been a host DoS,
>> which is substantially better than a guest escape.
> This seems quite sensible, and I'm glad Andy clarified.  (Although in
> a lot of these cases, a domain_crash() would be preferable to a
> BUG_ON()  if possible.)

Absolutely.  Clean error handling without a crash is certainly
preferable when possible.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel