Opened 2 years ago

Closed 23 months ago

#28592 closed defect (fixed)

Spurious coverity memory leak error in memoize_protover_summary()

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords:
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: teor Sponsor:

Description

Coverity says:

** CID 1441482:  Resource leaks  (RESOURCE_LEAK)
/src/core/or/versions.c: 402 in memoize_protover_summary()


________________________________________________________________________________________________________
*** CID 1441482:  Resource leaks  (RESOURCE_LEAK)
/src/core/or/versions.c: 402 in memoize_protover_summary()
396     {
397       if (!protover_summary_map)
398         protover_summary_map = strmap_new();
399     
400       if (strmap_size(protover_summary_map) >= MAX_PROTOVER_SUMMARY_MAP_LEN) {
401         protover_summary_cache_free_all();
   CID 1441482:  Resource leaks  (RESOURCE_LEAK)
   Overwriting "protover_summary_map" in "protover_summary_map = strmap_new()" leaks the storage that "protover_summary_map" points to.
402         protover_summary_map = strmap_new();
403       }
404     
405       const protover_summary_flags_t *cached =
406         strmap_get(protover_summary_map, protocols);     

But protover_summary_cache_free_all(); frees protover_summary_map.

cc'ing nickm, because the original fix was his.

Child Tickets

Change History (8)

comment:1 Changed 2 years ago by teor

Summary: Spurious coverity memory leak errorSpurious coverity memory leak error in memoize_protover_summary()

comment:2 Changed 2 years ago by rl1987

This is a false positive, as protover_summary_map is actually freed and assigned to NULL in protover_summary_cache_free_all(). I suspect macros in map.h make Coverity hard to analyze what's going on here.

My attempt at fixing this: https://github.com/torproject/tor/pull/544

comment:3 Changed 2 years ago by rl1987

Status: newneeds_review

comment:4 Changed 2 years ago by dgoulet

Reviewer: teor

comment:5 Changed 2 years ago by teor

Reviewer: teorteor, nickm

I think that asserting protover_summary_map == NULL is ok.

But I'd also like nickm's opinion: maybe there is a better way?

comment:6 Changed 23 months ago by teor

Status: needs_reviewmerge_ready

I think nickm will review this before it gets merged.
Is there a non-fatal alternative to the assert?

comment:7 Changed 23 months ago by teor

Reviewer: teor, nickmteor

comment:8 Changed 23 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

I think the assertion should be fine in this case, under the theory that it is about a single function doing what it is supposed to do. Merging!

Note: See TracTickets for help on using tickets.