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
