Opened 9 months ago

Closed 4 months ago

#29824 closed defect (fixed)

CID 1444119 resource leak in BUG case in consdiff.c

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: coverity, 035-backport, 040-backport, consider-backport-after-0405
Cc: Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: ahf Sponsor:

Description

*** CID 1444119:  Resource leaks  (RESOURCE_LEAK)
/src/feature/dircommon/consdiff.c: 1392 in consensus_diff_apply()
1386       int r1;
1387       char *result = NULL;
1388       memarea_t *area = memarea_new();
1389     
1390       r1 = consensus_compute_digest_as_signed(consensus, consensus_len, &d1);
1391       if (BUG(r1 < 0))
>>>     CID 1444119:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "area" going out of scope leaks the storage it points to.
1392         return NULL; // LCOV_EXCL_LINE
1393     
1394       lines1 = smartlist_new();
1395       lines2 = smartlist_new();
1396       if (consensus_split_lines(lines1, consensus, consensus_len, area) < 0)
1397         goto done;

This looks to be a leak if there is a BUG() condition, and the line is already marked LCOV_EXCL_LINE. Should we mark this as ignored or something in Coverity, or do we want to put cleanup code under that BUG() condition?

Child Tickets

Change History (11)

comment:1 Changed 9 months ago by rl1987

Status: newneeds_review

comment:2 Changed 9 months ago by teor

Appveyor failed due to #29500, I'll trigger a rebuild.

comment:3 Changed 9 months ago by asn

Reviewer: ahf

comment:4 Changed 9 months ago by ahf

Status: needs_reviewmerge_ready

Looks good.

comment:5 Changed 9 months ago by teor

Actual Points: 0.1
Keywords: asn-merge nickm-merge teor-merge consider-backport-after-0405-alpha 034-backport 035-backport 040-backport added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Points: 0.1
Version: Tor: 0.3.1.1-alpha

This is a low-risk change that can be backported to 0.3.4 and later, after it has been tested in at least one alpha.

Cherry-picked to maint-0.3.4 as:
https://github.com/torproject/tor/pull/848

Anyone can merge to 0.4.0 and merge forward to master after the CI passes.

comment:6 Changed 9 months ago by teor

Milestone: Tor: 0.4.1.x-finalTor: 0.3.5.x-final

Merged to 0.4.0 and later, marked for backport after testing in 0.4.0.4-alpha.

comment:7 Changed 8 months ago by asn

Keywords: asn-merge removed

Removing asn-merge tag given that this ticket is backport-candidate.

comment:8 Changed 8 months ago by teor

Keywords: nickm-merge teor-merge removed

I'll be the one who merges this, once it has had enough testing.

comment:9 Changed 8 months ago by teor

Keywords: consider-backport-after-0405 added; consider-backport-after-0405-alpha removed

Drop the -alpha from backport tags

comment:10 Changed 6 months ago by nickm

Keywords: 034-backport removed

Removing 034-backport from all open tickets: 034 has reached EOL.

comment:11 Changed 4 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Backported to 0.3.5.
Merged with the other 0.3.5 and 0.4.0 backports on 2019-08-12.

Note: See TracTickets for help on using tickets.