Opened 3 years ago

Closed 3 years ago

#20629 closed defect (fixed)

hs: Fix issues found by coverity

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor: SponsorR-must

Description

They've been mostly already fixed except on in the branch attached to this ticket.

However, that one is a mystery to me, in test_hs_descriptor.c:

206               char *addr1 = tor_addr_to_str_dup(&ls1->u.ap.addr),
207                    *addr2 = tor_addr_to_str_dup(&ls2->u.ap.addr);
>>>     CID 1375997:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "addr1" going out of scope leaks the storage it points to.
208               tt_str_op(addr1, OP_EQ, addr2);
209               tor_free(addr1);
210               tor_free(addr2);

(Same goes for addr2)

Is this about the fact that if tt_str_op() is triggered, we go to the done: label and then it leaks? I would be surprised as we often do that in test that is we assert and then we free.

Child Tickets

Change History (5)

comment:1 Changed 3 years ago by dgoulet

Status: newmerge_ready

See branch: bug20629_030_01. Going in merge_ready mode here because the fix is trivial and hopefully makes coverity happy.

For completion, here is what coverity was complaining about:

>>>     CID 1375998:  Null pointer dereferences  (REVERSE_INULL)
>>>     Null-checking "chunked_desc" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
1391       if (chunked_desc) {
1392         SMARTLIST_FOREACH(chunked_desc, char *, a, tor_free(a));
1393         smartlist_free(chunked_desc);
1394       }

comment:2 Changed 3 years ago by nickm

Is this about the fact that if tt_str_op() is triggered, we go to the done: label and then it leaks? I would be surprised as we often do that in test that is we assert and then we free.

Actually, we have to fix that all the time :)

Coverity will complain if there is any path where the memory is allocated but not freed.

comment:3 in reply to:  2 Changed 3 years ago by dgoulet

Replying to nickm:

Is this about the fact that if tt_str_op() is triggered, we go to the done: label and then it leaks? I would be surprised as we often do that in test that is we assert and then we free.

Actually, we have to fix that all the time :)

Big problem with this approach is that we can't scope our variables in {} which we do heavily at least in the HS tests. So we should make all of them global to the function and just free all of it in done:? If so, I can open a ticket just for that pass on the test.

Coverity will complain if there is any path where the memory is allocated but not freed.

I'm surprised that Coverity doesn't find _more_ of those as there are a lot.

comment:4 Changed 3 years ago by nickm

Merged. Please close if there are not more coverity issues to fix?

comment:5 Changed 3 years ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

I believe they are all done. I'll make it sure at the next coverity upload.

Note: See TracTickets for help on using tickets.