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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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.

To be clear: I'm fine with every way of deviating the construct, but 
agreeing on an
alternate mechanism to SAF-x-false-positive would land later than 
implementing some form
of error handling, I think.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)