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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.11.2023 14:28, Nicola Vetrini wrote:
> On 2023-11-08 12:19, Jan Beulich wrote:
>> On 08.11.2023 12:03, Nicola Vetrini wrote:
>>> On 2023-11-08 09:24, Jan Beulich wrote:
>>>> On 03.11.2023 18:58, 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.
>>>>
>>>> I disagree with this being a possible justification for adding such a
>>>> redundant assertion. More detail is needed on what is actually
>>>> (suspected to be) confusing the tool. Plus it also needs explaining
>>>> why (a) adding such an assertion helps and (b) how that's going to
>>>> cover release builds.
>>>>
>>>
>>> How about:
>>> "Static analysis tools may detect a possible null pointer dereference
>>> at line 760 (config->handle) due to config possibly being NULL.
>>>
>>> However, given that all system domains, including IDLE, have a NULL
>>> config and in the code path leading to the assertion only real domains
>>> (which have a non-NULL config) can be present."
>>>
>>> On point b): this finding is a false positive, therefore even if the
>>> ASSERT is
>>> expanded to effectively a no-op, there is no inherent problem with 
>>> Xen's
>>> code.
>>> The context in which the patch was suggested [1] hinted at avoiding
>>> inserting in
>>> the codebase false positive comments.
>>
>> Which I largely agree with. What I don't agree with is adding an
>> assertion which is only papering over the issue, and only in debug
>> builds. So perhaps instead we need a different way of tracking
>> false positives (which need to be tied to specific checker versions
>> anyway).
>>
> 
> Hmm. Is it better in your opinion to write something like:
> 
> if (config == NULL)
>     return ERR_PTR(<some error code>); // or die() or something 
> appropriate
> 
> this would be a rudimentary handling of the error with some messages 
> detailing that something
> is wrong if a domain has a null config at that point.

No. This is no better than a redundant assertion. It's even slightly
worse, as it'll likely leave a trace in generated code for release
builds.

Jan