1430749 - Update the list of checkers in config.yaml to add some clang-analyzer-security checks

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Andi [:andi]]

We want to add the following checkers to our default list:

 - clang-analyzer-security.FloatLoopCounter
 - clang-analyzer-security.insecureAPI.getpw
 - clang-analyzer-security.insecureAPI.gets
 - clang-analyzer-security.insecureAPI.mkstemp
 - clang-analyzer-security.insecureAPI.mktemp
 - clang-analyzer-security.insecureAPI.rand
 - clang-analyzer-security.insecureAPI.vfork

These checkers have proved to be very reliable with 0 margin of false positive.

Comment 1

[Andi [:andi]]

Attachment: Bug 1430749 - Update the list of checkers in config.yaml to add some clang-analyzer-security checks.

Review commit: https://reviewboard.mozilla.org/r/213154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213154/

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8942885 [details]
Bug 1430749 - Update the list of checkers in config.yaml to add some clang-analyzer-security checks.

https://reviewboard.mozilla.org/r/213154/#review218832

::: tools/clang-tidy/config.yaml:13
(Diff revision 1)
>     publish: !!bool no
>   - name: bugprone-forward-declaration-namespace
>     publish: !!bool yes
>   - name: clang-analyzer-deadcode.DeadStores
>     publish: !!bool yes
> - - name: clang-analyzer-security.*
> + - name: clang-analyzer-security.FloatLoopCounter

Nit: Why not the following, simpler config?

```
- name: clang-analyzer-security.*
  publish: !!bool yes
- name: -clang-analyzer-security.insecureAPI.strcpy
  publish: !!bool no
```

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8942885 [details]
Bug 1430749 - Update the list of checkers in config.yaml to add some clang-analyzer-security checks.

https://reviewboard.mozilla.org/r/213154/#review218834

R+ with 1 nit.

::: tools/clang-tidy/config.yaml:13
(Diff revision 1)
>     publish: !!bool no
>   - name: bugprone-forward-declaration-namespace
>     publish: !!bool yes
>   - name: clang-analyzer-deadcode.DeadStores
>     publish: !!bool yes
> - - name: clang-analyzer-security.*
> + - name: clang-analyzer-security.FloatLoopCounter

Hm, this wouldn't work with our current implementation:

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py?q=with+open%28mozpath.join%28self.topsrcdir%2C+%22tools%22%2C+%22clang-tidy%22%2C+%22config.yaml%22%29%29+as+f%3A&redirect_type=single#1713

::: tools/clang-tidy/config.yaml:15
(Diff revision 1)
>     publish: !!bool yes
>   - name: clang-analyzer-deadcode.DeadStores
>     publish: !!bool yes
> - - name: clang-analyzer-security.*
> + - name: clang-analyzer-security.FloatLoopCounter
> +   publish: !!bool yes
> + - name: clang-analyzer-security.insecureAPI.getpw

Nit: Please add all 9 security checkers explicitly then, including `security.insecureAPI.UncheckedReturn`:

https://clang-analyzer.llvm.org/available_checks.html#security_checkers

Comment 4

[Andi [:andi]]

Comment on attachment 8942885 [details]
Bug 1430749 - Update the list of checkers in config.yaml to add some clang-analyzer-security checks.

https://reviewboard.mozilla.org/r/213154/#review218838

::: tools/clang-tidy/config.yaml:13
(Diff revision 1)
>     publish: !!bool no
>   - name: bugprone-forward-declaration-namespace
>     publish: !!bool yes
>   - name: clang-analyzer-deadcode.DeadStores
>     publish: !!bool yes
> - - name: clang-analyzer-security.*
> + - name: clang-analyzer-security.FloatLoopCounter

yep, that's why i put them by hand. We could extend this functionality and if you choose publish: !!bool no then the published key will look like:

-clang-analyzer-security.insecureAPI.strcpy

Comment 5

[Andi [:andi]]

Comment on attachment 8942885 [details]
Bug 1430749 - Update the list of checkers in config.yaml to add some clang-analyzer-security checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213154/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9be7249e74fd
Update the list of checkers in config.yaml to add some clang-analyzer-security checks. r=janx

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9be7249e74fd