> On 19 Oct 2022, at 07:38, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 18.10.2022 18:11, Bertrand Marquis wrote:
>>> On 18 Oct 2022, at 16:29, Jan Beulich <jbeul...@suse.com> wrote:
>>> On 18.10.2022 17:17, Luca Fancellu wrote:
>>>>> On 13 Oct 2022, at 12:34, Jan Beulich <jbeul...@suse.com> wrote:
>>>>> On 13.10.2022 12:11, Luca Fancellu wrote:
>>>>>>> On 13 Oct 2022, at 08:50, Jan Beulich <jbeul...@suse.com> 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
