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). Jan
