Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#8793 closed defect (fixed)

Resolve clang scan-build issues

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, 024-backport, 025-triaged
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The clang analyzer tool "scan-build" reports a bunch of issues on current Tor master. On inspection, I believe that there's nothing harmful here. (Stuff is false-positive, or harmless, or both.) But we should nevertheless clean it all up if we can.

For extra points, we should integrate "scan-build" as part of our build hygiene process, and get jenkins to flip out when it doesn't pass error-free.

Child Tickets

Attachments (1)

scanbuild-code-review.txt (1.6 KB) - added by andrea 6 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by nickm

To run clang-analyzer on Tor, make sure you have it installed, then say something like "scan-build ./configure" then "make clean" and "scan-build make" in your Tor source. Add whatever configure or make arguments you need to. When it's done, it will tell you a directory to look at. Ignore the instructions to run scan-view on that directory, and just open it in your browser and look at index.html.

comment:3 Changed 6 years ago by nickm

Keywords: 025-triaged added

The false positive rate is too high here to make "fix all the issues" sensible. Somebody besides me should just confirm my impression that all the positives _are_ false positives, and then we should close the ticket.

comment:4 Changed 6 years ago by nickm

I've updated the above.

comment:5 Changed 6 years ago by nickm

I think we can safely skim through the dead assignment and dead increment warnings: those are mostly okay-style things.

comment:6 Changed 6 years ago by nickm

On review, I don't see any actual errors outside the tests, but I do see some functions that in isolation are wrong; some BUG messages that could crash if they ever triggered (in ways I think are impossible), and some code that could be more obviously correct.

I've started work on a branch to fix those. Then I'm going to re-run with all the interesting alpha checkers turned on and see whether it crashes.

comment:7 Changed 6 years ago by nickm

Owner: set to nickm
Status: newassigned

marking as assigned since I've started coding on these.

comment:8 Changed 6 years ago by nickm

Status: assignedneeds_review

Branch scanbuild_fixes in my public repository. I don't _think_ any of these bugs are hugely problematic, but more eyes on them could be a good idea.

comment:9 Changed 6 years ago by nickm

Keywords: 024-backport added

I suggest that these are backportable:

  • 3b1f7f75a7efa51ae5
  • 4d51dcda2fa75a384

Changed 6 years ago by andrea

Attachment: scanbuild-code-review.txt added

comment:10 Changed 6 years ago by andrea

Attached code review of nickm's scanbuild_fixes branch; per subsequent discussion I think this should be merged.

comment:11 Changed 6 years ago by nickm

  • 0fd0f5f7a9309fb90a6a4d8bad7d6399a45c7cc1 looks like a definite win
    • Could the bug it fixes ever arise under attacker control?

That depends on whether the LD_BUG messages it's fixing can actually occur. They're not in any released Tor yet, though: They affect code that was added for 9841, though, which hasn't been in any Tor release yet.

  • 4d51dcda2fa75a3841e041ab7c3de325d73e2850
    • But at least in theory we could have a hash table that large on a 64-bit platform
    • The numeric overflow would still be present because name##_PRIMES[] would still be an unsigned int.

Yes, but we multiply that uint by a size_t , which does an integer promotion. So it it won't overflow on 64-bit.

  • 9c9e07963dddff6e11330e9dc8ad7a6d37da4aa4
    • This patch looks okay but it overallocates slightly in the common case. Maybe malloc(len*2+ellipses+1) rather than malloc(len*2+4) ?

I'm okay wasting 3 bytes when reporting a tt_mem_op() failure in the tests.

comment:12 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

based on discussion, merging to 0.2.5 and marking for partial 0.2.4 backport.

comment:13 Changed 6 years ago by nickm

Recommendation : Partial backport as above

comment:14 Changed 6 years ago by nickm

Keywords: nickm-backport-02422 added

Adding a tag for tickets I think we should backport into 0.2.4.22. Omitting ones where I said "unsure"

comment:15 Changed 6 years ago by arma

So these are both issues that can't happen in the (current) code? Seems best to leave it alone then imo, and 0.2.5.x will be here soon enough.

comment:16 Changed 6 years ago by nickm

Those are both issues we think can't happen in the current code. I hope we're right about that, and I hope we don't backport anything weird.

comment:17 Changed 6 years ago by arma

All the more reason to avoid backporting things, just in case they're weird. :)

comment:18 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Resolution: fixed
Status: needs_reviewclosed

Okay, no backport for these.

comment:19 Changed 6 years ago by nickm

Keywords: nickm-backport-02422 removed
Note: See TracTickets for help on using tickets.