Opened 3 years ago

Last modified 5 months ago

#18897 new enhancement

Narrow scan-build checkers to those that have an acceptably low false positive rate.

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: automated-testing code-quality testing technical-debt
Cc: Actual Points:
Parent ID: #30225 Points: 1
Reviewer: Sponsor: SponsorS-can

Description

Our scan-build.sh script was written to use as many checkers as possible. But the false positive rate is unacceptably high -- to the point that we haven't routinized its use. We should instead try to get the false positive rate to the point where we are willing to run it daily and actually look at its output.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by nickm

Right now on master I see 29 bugs:

Cast region with wrong size 4 -- all false, I think, unless there's something wrong with char **x = tor_calloc(n, sizeof(char*)).

Out-of-bound access 18 -- basically any time we run a char* over an allocated memory buffer, this complains.

Out-of-bound array access 2 -- both in some rather fuzzy code in process_environment_make. Worth looking at more closely.

Use fixed address 2 -- Doesn't like our use of SIG_IGN.

Memory Error

Use-after-free 3 -- The reference-counting code in our handle logic seems to confuse the checker here. Or there is a bug that I just can't see.

comment:2 Changed 3 years ago by teor

By "daily basis" do you mean a make target, or also a jenkins run?

Tor passes the standard OS X Xcode "Deep Analyze" clang scan-build checks, or at least it did when I last ran them on an 0.2.8-alpha.

So let's aim for something between tor's current checks and the OS X "Deep Analyze" checks, which as far as I can tell, should be the default checks with: --mode deep.

That said, this will likely produce different results on different clang versions.

comment:3 in reply to:  2 Changed 3 years ago by teor

Replying to teor:

Tor passes the standard OS X Xcode "Deep Analyze" clang scan-build checks, or at least it did when I last ran them on an 0.2.8-alpha.

No, it's become smarter!

There are about 100 spurious "value stored is never read" warnings that appear to be good defensive programming practice. We should turn this type of warning off.

There's also a spurious "garbage data" warning in ge25519_scalarmult_base_niels because clang can't tell the size of a char * containing key data. I tried to eliminate this in 0.2.8(?), but clang got smarter and found it again. Not sure what to do about this.

comment:4 Changed 3 years ago by nickm

Points: small

comment:5 Changed 3 years ago by nickm

Keywords: 029-nickm-says-yes added

marking these as the tickets I am in favor of for 029, among the 029-proposed ones.

comment:6 Changed 3 years ago by nickm

Keywords: 029-proposed 029-nickm-says-yes removed
Milestone: Tor: 0.2.9.x-final

Nobody objected to including any of these, so I guess they are in.

comment:7 Changed 3 years ago by isabela

Points: small1

comment:8 Changed 3 years ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:9 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:10 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:11 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:12 Changed 2 years ago by nickm

Keywords: isaremoved removed

comment:13 Changed 2 years ago by nickm

Keywords: automated-testing code-quality testing technical-debt added

comment:14 Changed 5 months ago by teor

Parent ID: #30225
Note: See TracTickets for help on using tickets.