Opened 12 months ago

Last modified 5 months ago

#26780 needs_information defect

Fix division by zero error reports from Coverity (CID: 1415721, 1415722, and 1415723)

Reported by: ahf Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:

Description

We should fix the current Coverity issues. Coverity CID 1415721, 1415722, and 1415723 seems to be somewhat related in that they all stem from Coverity not being able to identify the output domain for get_net_param_from_list()

Child Tickets

Change History (8)

comment:1 Changed 12 months ago by ahf

comment:2 Changed 12 months ago by ahf

Status: newneeds_review

Looks like 1407.6 has been failing for a while on Travis. Marking this needs review anyway.

comment:3 Changed 12 months ago by dgoulet

Reviewer: asn

comment:4 Changed 12 months ago by asn

Status: needs_reviewmerge_ready

Changes LGTM.

I checked the coverity reports and I have no idea if the patch will fix them. I already imagine that coverity would not warn because of:

  if (res < min_val) {
    log_warn(LD_DIR, "Consensus parameter %s is too small. Got %d, raising to "
             "%d.", param_name, res, min_val);
    res = min_val;
  } else if (res > max_val) {
    log_warn(LD_DIR, "Consensus parameter %s is too large. Got %d, capping to "
             "%d.", param_name, res, max_val);
    res = max_val;
  }

Perhaps let's get these asserts merged, and check coverity again? :)

comment:5 Changed 12 months ago by nickm

Fine with me -- merged to master. Please close this if the coverity warnings go away.

comment:6 Changed 12 months ago by nickm

Hm. The warning is still happening. I've pushed another tweak as 9ae35975402a823a420cd5efb81a1c3a76f6c4d6 to see if that helps.

comment:7 Changed 12 months ago by nickm

(If it does help, the answer is to use the same approach elsewhere.)

comment:8 Changed 5 months ago by teor

Status: merge_readyneeds_information

We merged a fix to this ticket - we need to check if the coverity warnings went away.

Note: See TracTickets for help on using tickets.