Opened 5 months ago

Last modified 5 months ago

#26780 merge_ready 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 (7)

comment:1 Changed 5 months ago by ahf

comment:2 Changed 5 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 5 months ago by dgoulet

Reviewer: asn

comment:4 Changed 5 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 5 months ago by nickm

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

comment:6 Changed 5 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 5 months ago by nickm

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

Note: See TracTickets for help on using tickets.