Opened 3 years ago

Closed 3 years ago

#19571 closed defect (fixed)

Missing break statement in RelayNetworkStatusImpl

Reported by: karsten Owned by: iwakeh
Priority: Medium Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: iwakeh Actual Points:
Parent ID: #19398 Points:
Reviewer: Sponsor:

Description

Ugh, I just found a potential bug in the patch attached to #17824: The last switch case in RelayNetworkStatusImpl doesn't contain a break statement and hence falls through to the default case. This code is still in master. I didn't investigate yet what effects that has, but I feel it might break things.

Child Tickets

Attachments (1)

0002-Added-test-and-bugfix-for-RelayNetworkStatusImpl.patch (4.2 KB) - added by iwakeh 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by karsten

Parent ID: #19398

We should probably wait with the 1.3.0 release until this bug is fixed, well, if it's a bug after all.

comment:2 Changed 3 years ago by iwakeh

Hmm, there should be a (new) test failing for such a thing. It seems to be a bug.

If I should work on this (as it was my patch), just assign it to me.
Or, in case you're at it because of the other changes please set it to assigned.

comment:3 Changed 3 years ago by karsten

Owner: changed from karsten to iwakeh
Status: newassigned

Are you available to work on this? If not, I'll work on it tomorrow. Thanks!

comment:4 Changed 3 years ago by iwakeh

Owner: changed from iwakeh to karsten

Actually, I'm lacking the test data, i.e. a valid network-status v2 document.

Reassigning and curiously waiting to review again :-)

comment:5 Changed 3 years ago by karsten

Oh, what's wrong with the descriptors in CollecTor's archive? https://collector.torproject.org/archive/relay-descriptors/statuses/

But I can work on this now, unless you grab it back very quickly. ;)

comment:6 Changed 3 years ago by iwakeh

Owner: changed from karsten to iwakeh

comment:7 Changed 3 years ago by iwakeh

Status: assignedneeds_review

Thanks for the test data pointer :-)

patch attached, based on the earlier cobertura patch for #19574

comment:8 Changed 3 years ago by karsten

Patch looks good. Pushed to my task-19574 branch, together with the following CHANGELOG entry:

   - Stop reporting "-----END .*-----" lines in v2 network statuses as
     unrecognized.

If you prefer something else, let me know. Otherwise, I'll push to master and close tomorrow. Thanks!

comment:9 Changed 3 years ago by iwakeh

Status: needs_reviewmerge_ready

The changelog entry is fine.

I forgot to adapt the branch rate to 42% for cobertura check; it should be:

    <cobertura-check branchrate="0" totallinerate="31" totalbranchrate="42" >
      <regex pattern="org.torproject.descriptor.benchmark.*" branchrate="0" linerate="0"/>
    </cobertura-check>

comment:10 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

Changed the rate to 42 and pushed to master. Thanks! Closing.

Note: See TracTickets for help on using tickets.