Opened 5 months ago

Closed 4 months ago

#30150 closed defect (fixed)

Fix coverity warnings in tests

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 041-should, asn-merge
Cc: Actual Points: .1
Parent ID: #30146 Points:
Reviewer: dgoulet Sponsor:

Description

It's not really a problem if the unit tests could potentially leak memory or something, but in the interest of code quality, we should fix all the coverity warnings here too.

Child Tickets

Change History (10)

comment:1 Changed 4 months ago by nickm

Keywords: 041-should added

comment:2 Changed 4 months ago by nickm

Actual Points: .1
Status: assignedneeds_review

Most of these are easy, and I have tried to take care of them in coverity_in_tests with PR at https://github.com/torproject/tor/pull/1032 .

I have made a new ticket #30527 to handle the remaining cases, which are too tricky for a postfreeze series IMO.

comment:3 Changed 4 months ago by asn

Reviewer: dgoulet

comment:4 Changed 4 months ago by dgoulet

Status: needs_reviewneeds_information

This seems to mostly fix this pattern:

+  if (service) {
+    remove_service(get_hs_service_map(), service);
+    hs_service_free(service);
+  }
   hs_free_all

But hs_free_all goes over the service map and frees every entry so it is really not necessary to do so. The helper_create_service() in the HS tests always register the service to the global map.

comment:5 Changed 4 months ago by nickm

Status: needs_informationneeds_review

Right, but the issue there is that coverity can't tell that hs_free_all() frees that one, so we have tons of false positives.

comment:6 Changed 4 months ago by dgoulet

Status: needs_reviewmerge_ready

Hmmm ok so we do this pattern for coverity instead of flagging false positive in the coverity interface?

I'm fine either way.

comment:7 Changed 4 months ago by nickm

Keywords: asn-merge added

That's right. Our usual pattern is to make the coverity warnings go away whenever it is possible and doesn't make the code horrible.

comment:8 Changed 4 months ago by asn

There were conflicts with master here is an updated branch/PR: https://github.com/torproject/tor/pull/1059

comment:9 Changed 4 months ago by nickm

conflict-resolution commit LGTM.

comment:10 Changed 4 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged.

Note: See TracTickets for help on using tickets.