Re: [v2] Proposal for deviations in static analyser findings

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 26 Oct 2022, at 10:36, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 25/10/2022 11:13, Luca Fancellu wrote:
>> Hi all,
> 
> Hi Luca,
> 
> Some comments below if we plan to merge the doc in the tree.
> 
>> This is the V2 of the proposal for deviations tagging in the Xen codebase, 
>> this includes
>> all the feedbacks from the FuSa session held at the Xen Summit 2022 and all 
>> the
>> feedbacks received in the previous proposal sent on the mailing list.
>> Here a link to the previous thread:
>> https://lists.xenproject.org/archives/html/xen-devel/2022-10/msg00541.html >
>> Documenting violations
>> ======================
>> Static analysers are used on the Xen codebase for both static analysis and 
>> MISRA
>> compliance.
>> There might be the need to suppress some findings instead of fixing them and
>> many tools permit the usage of in-code comments that suppress findings so 
>> that
>> they are not shown in the final report.
>> Xen will include a tool capable of translating a specific comment used in its
>> codebase to the right proprietary in-code comment understandable by the 
>> selected
>> analyser that suppress its finding.
>> In the Xen codebase, these tags will be used to document and suppress 
>> findings:
>> - SAF-X-safe: This tag means that the next line of code contains a finding, 
>> but
>>  the non compliance to the checker is analysed and demonstrated to be safe.
>> - SAF-X-false-positive-<tool>: This tag means that the next line of code 
>> contains a
>>  finding, but the finding is a bug of the tool.
>> SAF stands for Static Analyser Finding, the X is a placeholder for a positive
>> number, the number after SAF- shall be incremental and unique, base ten
>> notation and without leading zeros.
>> 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.
>> An entry can be reused in multiple places in the code to suppress a finding 
>> if
>> and only if the justification holds for the same non-compliance to the coding
>> standard.
>> An orphan entry, that is an entry who was justifying a finding in the code, 
>> but later
>> that code was removed and there is no other use of that entry in the code, 
>> can be
>> reused as long as the justification for the finding holds. This is done to 
>> avoid the
>> allocation of a new entry with exactly the same justification, that would 
>> lead to waste
>> of space and maintenance issues of the database.
>> The files where to store all the justifications are in xen/docs/misra/ and 
>> are
>> named as safe.json and false-positive-<tool>.json, they have JSON format, 
>> entries
>> of these files have independent ID numbering.
>> Here is an example to add a new justification in safe.json::
>> |{
>> |    "version": "1.0",
>> |    "content": [
>> |        {
>> |            "id":"SAF-0-safe",
>> |            "analyser": {
>> |                "cppcheck": "misra-c2012-20.7",
>> |                "coverity": "misra_c_2012_rule_20_7_violation",
>> |                "eclair": "MC3R1.R20.7"
>> |            },
>> |            "name": “R20.7 C macro parameters not used as expression",
>> |            "text": "The macro parameters used in this […]"
>> |        },
>> |        {
>> |            "id":”SAF-1-safe",
>> |            "analyser": {
>> |                "cppcheck": "unreadVariable",
>> |                "coverity": "UNUSED_VALUE"
>> |            },
>> |            "name": “Variable set but not used",
>> |            "text": “It is safe because […]"
>> |        },
>> |        {
>> |            "id":”SAF-2-safe",
>> |            "analyser": {},
>> |            "name": "Sentinel",
>> |            "text": ""
>> |        }
>> |    ]
>> |}
>> Here is an example to add a new justification in 
>> false-positive-cppcheck.json::
>> |{
>> |    "version": "1.0",
>> |    "content": [
>> |        {
>> |            "id":"SAF-0-false-positive-cppcheck",
>> |            "analyser": {
>> |                "cppcheck": "misra-c2012-20.7"
>> |            },
>> |            “tool-version”: “2.7",
>> |            "name": “R20.7 second operand of member-access operator",
>> |            "text": "The second operand of a member access operator shall 
>> be a name of a member of the type pointed to, so in this particular case it 
>> is wrong to use parentheses on the macro parameter."
>> |        },
>> |        {
>> |            "id":”SAF-1-false-positive-cppcheck",
>> |            "analyser": {},
>> |            “tool-version”: “",
>> |            "name": "Sentinel",
>> |            "text": ""
>> |        }
>> |    ]
>> |}
>> To document a finding, just add another block {[...]} before the sentinel 
>> block,
>> using the id contained in the sentinel block and increment by one the number
>> contained in the id of the sentinel block.
>> Here a brief explanation of the field inside an object of the "content" 
>> array:
> 
> You don't seem to have a longer explanation afterwards. So I would drop 
> "brief".

Ok will remove it

> 
>> - id: it is a unique string that is used to refer to the finding, many 
>> finding
>>  can be tagged with the same id, if the justification holds for any applied
>>  case.
>>  It tells the tool to substitute a Xen in-code comment having this structure:
>>  /* SAF-0-safe [...] \*/
>> - analyser: it is an object containing pair of key-value strings, the key is
>>  the analyser, so it can be cppcheck, coverity or eclair. The value is the
>>  proprietary id corresponding on the finding, for example when coverity is
>>  used as analyser, the tool will translate the Xen in-code coment in this 
>> way:
>>  /* SAF-0-safe [...] \*/ -> /* coverity[coverity-id] \*/
>>  if the object doesn't have a key-value, then the corresponding in-code
>>  comment won't be translated.
>> - name: a simple name for the finding
>> - text: a proper justification to turn off the finding.
>> Here an example of the usage of the in-code comment tags to suppress a 
>> finding for the Rule 8.6:
>> Eclair reports it here:
>> https://eclairit.com:3787/fs/var/lib/jenkins/jobs/XEN/configurations/axis-Target/ARM64/axis-agent/public/builds/549/archive/ECLAIR/out/PROJECT.ecd;/sources/xen/include/xen/kernel.h.html#R50743_1
> 
> How stable is this link?

At first I thought to don’t put in the document the example, so I would have 
dropped from “Here an example of the usage of the in-code […]”,
but I understand I was not very clear on that, and I see the example could be 
also useful, so I think I can create a section in the document
like this below and I would drop the link in favour of some text from eclair 
report.


Justification example
---------------------

Here an example of the usage of the in-code comment tags to suppress a finding
for the Rule 8.6:

Eclair reports it in its report:

| [elcair report] (now eclair is unreachable for an internal error, I will take 
the output when it will be back)
| [...]

Also coverity reports it, here an extract of the finding:

| xen/include/xen/kernel.h:68:
| 1. misra_c_2012_rule_8_6_violation: Function "_start" is declared but never
 defined.

The analysers are complaining because we have this in xen/include/xen/kernel.h
at line 68::

| extern char _start[], _end[], start[];

Those are symbols exported by the linker, hence we will need to have a proper
deviation for this finding.

We will prepare our entry in the database::

|{
|    "version": "1.0",
|    "content": [
|        {
|        [...]
|        },
|        {
|            "id": "SAF-1-safe",
|            "analyser": {
|                “eclair": "MC3R1.R8.6",
|                "coverity": "misra_c_2012_rule_8_6_violation"
|            },
|            "name": "Rule 8.6: linker defined symbols",
|            "text": "It is safe to declare this symbol because it is defined 
in the linker script."
|        },
|        {
|            "id": "SAF-2-safe",
|            "analyser": {},
|            "name": "Sentinel",
|            "text": ""
|        }
|    ]
|}

And we will use the proper tag above the violation line::

| /* SAF-1-safe [optional text] */
| extern char _start[], _end[], start[];

This entry will fix also the violation on _end and start, because they are on
the same line and the same "violation ID".

Also, the same tag can be used on other symbols from the linker that are
declared in the codebase, because the justification holds for them too.


> 
> The rest of the document LGTM.
> 
> Cheers,
> 
> -- 
> Julien Grall