Opened 2 months ago

Last modified 6 weeks ago

#29824 merge_ready defect

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, 034-backport, 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 (9)

comment:1 Changed 2 months ago by rl1987

Status: newneeds_review

comment:2 Changed 2 months ago by teor

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

comment:3 Changed 2 months ago by asn

Reviewer: ahf

comment:4 Changed 2 months ago by ahf

Status: needs_reviewmerge_ready

Looks good.

comment:5 Changed 2 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 2 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 weeks ago by asn

Keywords: asn-merge removed

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

comment:8 Changed 8 weeks 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 6 weeks ago by teor

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

Drop the -alpha from backport tags

Note: See TracTickets for help on using tickets.