Opened 6 months ago

Closed 6 months ago

#29823 closed defect (fixed)

CID 1444117, CID 1444118 resource leaks in test_shared_random.c

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.1-alpha
Severity: Normal Keywords: coverity, tor-tests-unit, teor-merge, nickm-merge
Cc: Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: asn Sponsor:

Description

** CID 1444117:    (RESOURCE_LEAK)
/src/test/test_shared_random.c: 1098 in test_utils_general()
/src/test/test_shared_random.c: 1099 in test_utils_general()


________________________________________________________________________________________________________
*** CID 1444117:    (RESOURCE_LEAK)
/src/test/test_shared_random.c: 1098 in test_utils_general()
1092           "1BDB7C3E973936E4D13A49F37C859B3DC69C429334CF9412E3FEF6399C52D47A";
1093         srv = tor_malloc_zero(sizeof(*srv));
1094         srv->num_reveals = 42;
1095         memcpy(srv->value, srv_value, sizeof(srv->value));
1096         dup_srv = sr_srv_dup(srv);
1097         tt_assert(dup_srv);
>>>     CID 1444117:    (RESOURCE_LEAK)
>>>     Variable "dup_srv" going out of scope leaks the storage it points to.
1098         tt_u64_op(dup_srv->num_reveals, OP_EQ, srv->num_reveals);
1099         tt_mem_op(dup_srv->value, OP_EQ, srv->value, sizeof(srv->value));
1100         tor_free(srv);
1101         tor_free(dup_srv);
1102       }
1103     
/src/test/test_shared_random.c: 1099 in test_utils_general()
1093         srv = tor_malloc_zero(sizeof(*srv));
1094         srv->num_reveals = 42;
1095         memcpy(srv->value, srv_value, sizeof(srv->value));
1096         dup_srv = sr_srv_dup(srv);
1097         tt_assert(dup_srv);
1098         tt_u64_op(dup_srv->num_reveals, OP_EQ, srv->num_reveals);
>>>     CID 1444117:    (RESOURCE_LEAK)
>>>     Variable "dup_srv" going out of scope leaks the storage it points to.
1099         tt_mem_op(dup_srv->value, OP_EQ, srv->value, sizeof(srv->value));
1100         tor_free(srv);
1101         tor_free(dup_srv);
1102       }
1103     
1104       /* Testing commitments_are_the_same(). Currently, the check is to test the
** CID 1444118:    (RESOURCE_LEAK)
/src/test/test_shared_random.c: 1098 in test_utils_general()
/src/test/test_shared_random.c: 1099 in test_utils_general()


________________________________________________________________________________________________________
*** CID 1444118:    (RESOURCE_LEAK)
/src/test/test_shared_random.c: 1098 in test_utils_general()
1092           "1BDB7C3E973936E4D13A49F37C859B3DC69C429334CF9412E3FEF6399C52D47A";
1093         srv = tor_malloc_zero(sizeof(*srv));
1094         srv->num_reveals = 42;
1095         memcpy(srv->value, srv_value, sizeof(srv->value));
1096         dup_srv = sr_srv_dup(srv);
1097         tt_assert(dup_srv);
>>>     CID 1444118:    (RESOURCE_LEAK)
>>>     Variable "srv" going out of scope leaks the storage it points to.
1098         tt_u64_op(dup_srv->num_reveals, OP_EQ, srv->num_reveals);
1099         tt_mem_op(dup_srv->value, OP_EQ, srv->value, sizeof(srv->value));
1100         tor_free(srv);
1101         tor_free(dup_srv);
1102       }
1103     
/src/test/test_shared_random.c: 1099 in test_utils_general()
1093         srv = tor_malloc_zero(sizeof(*srv));
1094         srv->num_reveals = 42;
1095         memcpy(srv->value, srv_value, sizeof(srv->value));
1096         dup_srv = sr_srv_dup(srv);
1097         tt_assert(dup_srv);
1098         tt_u64_op(dup_srv->num_reveals, OP_EQ, srv->num_reveals);
>>>     CID 1444118:    (RESOURCE_LEAK)
>>>     Variable "srv" going out of scope leaks the storage it points to.
1099         tt_mem_op(dup_srv->value, OP_EQ, srv->value, sizeof(srv->value));
1100         tor_free(srv);
1101         tor_free(dup_srv);
1102       }
1103     
1104       /* Testing commitments_are_the_same(). Currently, the check is to test the

These look to be only leaks in assertion-failure cases. Maybe make the pointers in question function-scope, or break stuff out into helper functions so the done label can properly clean up?

Child Tickets

Change History (4)

comment:1 Changed 6 months ago by rl1987

Status: newneeds_review

comment:2 Changed 6 months ago by asn

Reviewer: asn

comment:3 Changed 6 months ago by asn

Status: needs_reviewmerge_ready

LGTM! Thanks!

comment:4 Changed 6 months ago by teor

Actual Points: 0.1
Keywords: teor-merge nickm-merge added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Points: 0.1
Resolution: fixed
Status: merge_readyclosed
Version: Tor: 0.2.9.1-alpha

This is a significant refactor in the unit tests.

It only affects resource deallocation on failure, so there isn't much point in backporting it. Once we've failed a unit test, further failures don't really matter.

Merged #29823 and #29789 to master.

Note: See TracTickets for help on using tickets.