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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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)