Closed Bug 1430749 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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
```
Attachment #8942885 - Flags: review?(janx)
Assignee: nobody → bpostelnicu
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
Attachment #8942885 - Flags: review+
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
Pushed by bpostelnicu@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/9be7249e74fd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Core → Firefox Build System
Depends on: 1451316
You need to log in before you can comment on or make changes to this bug.