#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:


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 13 months 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 13 months ago by asn

Reviewer: asn

comment:3 Changed 13 months 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 13 months ago by nickm

Keywords: dgoulet-merge added

comment:5 Changed 12 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.