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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 08/11/2023 11: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.
I expect that the number of issues will increase a lot as soon as we 
start to analyze production builds.
I don't think it will be a solution to either replace all the ASSERT() 
with runtime check in all configuration or even...
> So perhaps instead we need a different way of tracking
> false positives (which need to be tied to specific checker versions
> anyway).
... documenting false positive.

IMHO, the only viable option would be to have a configuration to keep 
ASSERT in production build for scanning tools.
Cheers,

--
Julien Grall