Re: Proposal for deviations in static analyser findings

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 19 Oct 2022, at 07:38, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 18.10.2022 18:11, Bertrand Marquis wrote:
>>> On 18 Oct 2022, at 16:29, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 18.10.2022 17:17, Luca Fancellu wrote:
>>>>> On 13 Oct 2022, at 12:34, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> On 13.10.2022 12:11, Luca Fancellu wrote:
>>>>>>> On 13 Oct 2022, at 08:50, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>>> On 12.10.2022 18:00, Luca Fancellu wrote:
>>>>>>>> Entries in the database should never be removed, even if they are not 
>>>>>>>> used
>>>>>>>> anymore in the code (if a patch is removing or modifying the faulty 
>>>>>>>> line).
>>>>>>>> This is to make sure that numbers are not reused which could lead to 
>>>>>>>> conflicts
>>>>>>>> with old branches or misleading justifications.
>>>>>>> 
>>>>>>> Can we add provisions for shrinking such entries to e.g. just their "id"
>>>>>>> line? Or is the intention to be able to re-use such an entry if a 
>>>>>>> matching
>>>>>>> instance appears again later?
>>>>>> 
>>>>>> I prefer to don’t shrink it, the name itself is not very long, even 
>>>>>> using many digits of the incremental
>>>>>> number, it removes also the dependency on the file name.
>>>>> 
>>>>> Name length isn't relevant here, and I have no idea what dependency on a
>>>>> file name you're thinking of. My question is a scalability one: Over time
>>>>> the table will grow large. If all entries remain there in full forever,
>>>>> table size may become unwieldy.
>>>> 
>>>> Ok I misunderstood your question, now I understand what you are asking, we 
>>>> could remove the content
>>>> of the “analyser” dictionary for sure, because if there is not anymore a 
>>>> link with the current code.
>>>> 
>>>> Regarding removing the “name” and “text”, could it be that at some point 
>>>> we can introduce in the code
>>>> a violation that requires the same justification provided by the “orphan” 
>>>> entry?
>>>> In that case we could reuse that entry without creating a new one that 
>>>> will only waste space.
>>>> What is the opinion on this?
>>> 
>>> Well, yes, this is the one case that I, too, was wondering about. It's not
>>> clear to me whether it wouldn't be better to allocate a fresh ID in such a
>>> case.
>> 
>> For traceability and release handling I think it is important that:
>> - we never reuse the same ID in any case
>> - we keep IDs in the database even if there is no occurrence in xen code as 
>> this will prevent adding a new ID if an existing one can be reused
>> 
>> In a certification context, when a justification has been validated and 
>> agreed it will make life a lot easier to not modify it and reuse it in the 
>> future if it is needed.
>> From our point of view, having a clear and simple way of handling those will 
>> make backports a lot easier.
> 
> Isn't validation of a justification connected to the affected code? If so,
> every new instance will need validation, while an orphan entry is entirely
> meaningless.
> 
>>>>>> After the analysis, the source code will return as the original (with 
>>>>>> the SAF-* tag).
>>>>> 
>>>>> While you mention something similar also as step 3 in the original 
>>>>> document
>>>>> near the top, I'm afraid I don't understand what this "return as the 
>>>>> original"
>>>>> means. If you want to run the tool on an altered (comments modified) 
>>>>> source
>>>>> tree, what I'd expect you to do is clone the sources into a throw-away 
>>>>> tree,
>>>>> massage the comments, run the tool, and delete the massaged tree.
>>>>>>>> if the object doesn't have a key-value, then the corresponding in-code
>>>>>>>> comment won't be translated.
>>>>>>> 
>>>>>>> Iirc at least Coverity ignores certain instances of what it might 
>>>>>>> consider
>>>>>>> violations (fall-through in switch() statements in particular) in case
>>>>>>> _any_ comment is present. Therefore may I suggest that such comments be
>>>>>>> deleted (really: replaced by a blank line, to maintain correct line
>>>>>>> numbering) if there's no matching key-value pair?
>>>>>> 
>>>>>> Yes the line won’t be altered if there is no match. This to ensure the 
>>>>>> correct line
>>>>>> numbering is not affected.
>>>>> 
>>>>> "won't be altered" is the opposite of what I've been asking to consider:
>>>>> Observing that comments _regardless_ of their contents may silence 
>>>>> findings,
>>>>> the suggestion is to remove comments (leaving a blank line) when there's 
>>>>> no
>>>>> entry for the targeted tool in the table entry.
>>>> 
>>>> Why? The tag comment won’t do anything, it would act as a blank line from 
>>>> the analyser
>>>> perspective.
>>> 
>>> The _tag_ won't do anything, but as said any _comment_ may have an effect.
>> 
>> I am not sure I follow this one but in any case we can choose to anyway 
>> substitute the tag with something like /* Not applicable */.
> 
> That's still a comment, which hence may still silence a tool:
> 
>    switch ( x )
>    {
>    case a:
>        ...
>        /* SAF-<N>-safe */
>    case b:
>        ...
>        break;
>    }
> 
> is no different from
> 
>    switch ( x )
>    {
>    case a:
>        ...
>        /* fall-through */
>    case b:
>        ...
>        break;
>    }
> 
> nor
> 
>    switch ( x )
>    {
>    case a:
>        ...
>        /* Not applicable */
>    case b:
>        ...
>        break;
>    }
> 
> Only
> 
>    switch ( x )
>    {
>    case a:
>        ...
> 
>    case b:
>        ...
>        break;
>    }
> 
> will make e.g. Coverity actually point out the potentially unintended
> fall through (based on past observations). Whether that behavior is
> fall-through-specific I don't know. If it indeed is, then maybe my
> concern is void - in the long run I think we want to use the pseudo-
> keyword there in all cases anyway.

https://www.synopsys.com/blogs/software-security/gimme-a-break/

Now I understand what you mean, the fall through is very difficult to 
understand from the tool because
It could be a source of a lot of false positive.

Hence Coverity came up with this, however I really doubt Coverity is silencing 
the finding on _any_ comment,
think about this example:

switch (x)
{
case a:
     <statement>
     /* break; */
case 2:
     <statement>
     break;
}

This is clearly a developer mistake and should not silence the finding: 
false-negative are way more harmful than
false-positive that are mostly annoying.

In fact I think Coverity is checking the content of the comment looking for 
fall-through/fall through/fall thru […]



> 
> Jan