Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#4637 closed defect (fixed)

Two coverity deadcode issues

Reported by: Sebastian Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Fixes coming soon

Child Tickets

Change History (12)

comment:1 Changed 9 years ago by Sebastian

Status: newneeds_review

Branch coverity in my repo. I think we shouldn't backport this, it isn't serious, and coverity doesn't check the maint branches anyway.

comment:2 Changed 9 years ago by nickm

Cc: mikeperry added

Agreed that we shouldn't backport.

Can we get Mike to review the weight-calculation part?

comment:3 Changed 9 years ago by mikeperry

I just looked at cid 433 commit. Interesting. What makes coverity think this is dead code? If for some reason the Guard flag assignment code choses not to assign any Guard flags to Exits, then we would have divide by zero errors here, and crash upon consensus formation.

Maybe coverity realized that it was impossible for us to do that with the current Guard assignment algorithm, but it is possible we might decide to do it in the future. Guard+Exit does put quite a TCP socket load on fast nodes...

comment:4 Changed 9 years ago by mikeperry

I guess I sort of trailed off there, but my opinion is the cid 433 commit probably should not be merged. It just leaves a hazard for us should we decide to change Guard assignment so that Exits don't get the Guard flag.

comment:5 Changed 9 years ago by Sebastian

No, that's not it. Just a couple of lines before all the checks, we have an explicit check already:

   if (G <= 0 || M <= 0 || E <= 0 || D <= 0) {
     log_warn(LD_DIR, "Consensus with empty bandwidth: "
                      "G="I64_FORMAT" M="I64_FORMAT" E="I64_FORMAT
                      " D="I64_FORMAT" T="I64_FORMAT,
              I64_PRINTF_ARG(G), I64_PRINTF_ARG(M), I64_PRINTF_ARG(E),
              I64_PRINTF_ARG(D), I64_PRINTF_ARG(T));
     return;
   }

This check was introduced after the explicit checks for 0 were already there, and they just weren't removed.

comment:6 Changed 9 years ago by mikeperry

That would do it then.

I still don't really like removing the D == 0 cases, as it seems like something we might want to support later... I suppose we could ifdef them out with a comment if we just want to silence coverity?

comment:7 Changed 9 years ago by Sebastian

Alternatively I could go ahead and mark it as intentional on coverity

comment:8 Changed 9 years ago by nickm

I'm fine with marking it intentional; deadcode is not a big problem, especially if the deadcode is checking for exceptional conditions.

comment:9 Changed 9 years ago by Sebastian

ok. Updating the branch to only contain the other commit then, and editing the changes file.

comment:10 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged; thanks!

comment:11 Changed 8 years ago by nickm

Keywords: tor-client added

comment:12 Changed 8 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.