Opened 7 weeks ago

Closed 4 weeks ago

#31025 closed defect (fixed)

Coverity is confused by switch statement in siphash24 implementation

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: coverity dgoulet-merge
Cc: Actual Points: .1
Parent ID: Points:
Reviewer: asn Sponsor:

Description

Even when siphash24() is called with a constant src_len, coverity seems to think we might be indexing beyond that length. My guess is that coverity can't handle the switch statement syntax that we're using.

This is CID 1447293 and CID 1447295.

Child Tickets

Change History (5)

comment:1 Changed 7 weeks ago by nickm

Actual Points: .1
Status: assignedneeds_review

https://github.com/torproject/tor/pull/1153 (ticket31025) is the best I could come up with here: using a slightly different byte-copy implementation when building for coverity.

I tried switching to this implementation unconditionally, but it caused a performance regression in siphash.

I do not know for certain if this will shut coverity up or not. :)

Another alternative might be writing a coverity model for the siphash24 function. I don't actually know how to do that correctly, though.

comment:2 Changed 7 weeks ago by asn

Reviewer: asn

comment:3 Changed 7 weeks ago by asn

Status: needs_reviewmerge_ready

LGTM! And the siphash unittest also seem to pass if I toggle the coverity flag.

Let's get this merged and see if it fixes our coverity false positives.

comment:4 Changed 7 weeks ago by nickm

Keywords: dgoulet-merge added

comment:5 Changed 4 weeks ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.