Re: [XEN PATCH][for-4.19] domain: add ASSERT to help static analysis tools

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/11/2023 1:45 pm, Nicola Vetrini wrote:
> On 2023-11-08 14:37, Andrew Cooper wrote:
>> On 03/11/2023 5:58 pm, Nicola Vetrini wrote:
>>> Static analysis tools may detect a possible null
>>> pointer dereference at line 760 (the memcpy call)
>>> of xen/common/domain.c. This ASSERT helps them in
>>> detecting that such a condition is not possible
>>> and also provides a basic sanity check.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>> ---
>>> The check may be later improved by proper error checking
>>> instead of relying on the semantics explained here:
>>> https://lore.kernel.org/xen-devel/61f04d4b-34d9-4fd1-a989-56b042b4f3d8@xxxxxxxxxx/
>>>
>>>
>>> This addresses the caution reported by ECLAIR for MISRA C:2012 D4.11
>>> ---
>>>  xen/common/domain.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index 8f9ab01c0cb7..9378c0417645 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -700,6 +700,8 @@ struct domain *domain_create(domid_t domid,
>>>
>>>      if ( !is_idle_domain(d) )
>>>      {
>>> +        ASSERT(config);
>>> +
>>>          watchdog_domain_init(d);
>>>          init_status |= INIT_watchdog;
>>>
>>
>> I have an idea that might resolve this differently and in an easier way.
>>
>> Would you be happy waiting for a couple of days for me to experiment? 
>> Absolutely no guarantees of it turning into a workable solution.
>>
>
> Sure, no problem.
>

I'm afraid my experiments have failed.  I've got a bit of cleanup done
(which does remove the idle-domain predicate in context above), but
nothing that I expect would help with this issue specifically.

The best I can suggest is to copy x86's arch_domain_create() in it's
handling of config, which would end up looking like:

if ( !config )
{
    ASSERT_UNREACHABLE();
    goto fail;
}

to make a runtime-failsafe path in the same pattern that we use
elsewhere, and is known to influence toolchains.

This is actually the pattern used to emulate __builtin_assume() in GCC. 
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf

~Andrew