1554655 - Skip third party files in ./mach static-analysis check

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Bastien Abadie [:bastien]]

The code review checks if a file is a 3rd party by downloading the tools/rewriting/ThirdPartyPaths.txt file from mozilla-central and matching its paths against the patch being analyzed.

This behaviour should be internal to ./mach static-analysis check, as it relies on a resource in the same repository.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Do we really want to skip checks in any of the code we build&ship?

I don't have experience downloading new/updated code, but I had some experience with fixing some imported code that had huge security holes. So I think we should be extra careful, and I would like to think that we're throwing every possible tool at code we haven't written, including static analysis.

What about the situation where non-imported code does something wrong with imported code, and a static analyzer could flag that, but won't because the warning would originate in the imported code?

I would be interested in knowing why you want to remove these checks. (Guessing there were too many false positives in some 3rd party code we can't easily modify?) Could we find other solutions?

And if we end up skipping checks, I think we should have a proper process in place, which strongly recommends that the importer/maintainer runs these checks at least once before committing the code.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

In perfect world, yeah, you would be 100 % correct Gérald. Coverity and the other tools find a number of defects on these project.

However, from our experience, we don't do much wrt third parties software. For a few reasons:

• dealing with backlog is hard and not fun
• you need to understand the code
• forwarding bugs aren't always easy (ex: icu was a pain but now that they moved to github, it might be easier)
• some defects are coding styles (example: else after a return), some others defects, etc

Now, we could only show new defects of a certain category.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Example: https://phabricator.services.mozilla.com/D30640

Comment 4

[Bastien Abadie [:bastien]]

Judging by the numerous issues on our projects related to third party files, it would not be a good idea to analyze those files.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Or just enable some specific rules (security, crash) of Coverity for them