Opened 2 months ago

Closed 4 weeks 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:


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 6 weeks ago by nickm

Keywords: 041-should added

comment:2 Changed 6 weeks 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 .

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

comment:3 Changed 5 weeks ago by asn

Reviewer: dgoulet

comment:4 Changed 5 weeks 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);
+  }

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 5 weeks 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 5 weeks 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 weeks 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 weeks ago by asn

There were conflicts with master here is an updated branch/PR:

comment:9 Changed 4 weeks ago by nickm

conflict-resolution commit LGTM.

comment:10 Changed 4 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.