Opened 4 years ago

Closed 4 years ago

#17753 closed defect (fixed)

Use internal reputation variables to verify memory allocation

Reported by: cypherpunks Owned by:
Priority: Low Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Minor Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The reputation code (rephist.c) has internal variables to keep track on the memory allocated by its objects. However, when the bandwidth arrays are freed, it forgets to decrease the memory allocation counter. This leads to discrepancy between the recorded and the actual amount of memory that is allocated when the bandwidth arrays are reinitialized.

Child Tickets

Attachments (1)

0001-Assert-that-memory-held-by-rephist-is-freed.patch (3.3 KB) - added by cypherpunks 4 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 4 years ago by cypherpunks

Status: newneeds_review

comment:2 Changed 4 years ago by teor

Milestone: Tor: 0.2.8.x-final
Status: needs_reviewneeds_revision

This patch looks good - thanks for making sure we don't leak this memory.

Can you explain the removal of the cast from the line:
lhist = (link_history_t*) digestmap_get(orhist->link_history_map, to_id);

Did you mean to include it in the patch?

comment:3 in reply to:  2 ; Changed 4 years ago by cypherpunks

Replying to teor:

Can you explain the removal of the cast from the line:
lhist = (link_history_t*) digestmap_get(orhist->link_history_map, to_id);

As explained in the commit message, i found this unnecessary cast while working on the code. The digestmap returns a pointer to void so it automatically converts to whatever pointer type you assign it to already, no cast necessary.

Did you mean to include it in the patch?

I did mean to include it in the patch because it was such a small change, but i agree that it is a bit unrelated. Should i split the commit in two?

Last edited 4 years ago by cypherpunks (previous) (diff)

comment:4 in reply to:  3 Changed 4 years ago by teor

Status: needs_revisionneeds_review

Replying to cypherpunks:

Replying to teor:

Can you explain the removal of the cast from the line:
lhist = (link_history_t*) digestmap_get(orhist->link_history_map, to_id);

As explained in the commit message, i found this unnecessary cast while working on the code. The digestmap returns a pointer to void so it automatically converts to whatever pointer type you assign it to already, no cast necessary.

Thanks for picking this up, it's the only location in the entire codebase that we cast the result of digestmap_get. (For those circumstances where casting is necessary, there are automatically generated functions that cast to a type.)

Did you mean to include it in the patch?

I did mean to include it in the patch because it was such a small change, but i agree that it is a bit unrelated. Should i split the commit in two?

There's no need to split this commit.
(In future, please keep one commit per topic. It's easier for reviewers to understand.)

Thanks for the patch, I am happy with it. Let's get it merged.

comment:5 Changed 4 years ago by dgoulet

+1 with teor here on the split but else this patch lgtm also!

comment:6 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

lgtm too; merging.

Note: See TracTickets for help on using tickets.