Opened 4 years ago

Closed 4 years ago

#16924 closed defect (fixed)

Properly free channel during channel_free_list

Reported by: Sebastian Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: 026-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Branch channel_free_list in my repository. We have to take care not to confuse the smartlist during SMARTLIST_FOREACH.

Child Tickets

Change History (7)

comment:1 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

It's okay to do it the way that channel_free_list() does it now, so long as the list isn't used again. So that's fine. The issue here is that the function doesn't document that it invalidates the lists. I'd prefer a documentation fix for this, and possibly doing a smartlist_clear() at the end of the functions. Also see channel_listener_free_list().

Please let me know if I messed up the analysis here!

comment:2 Changed 4 years ago by Sebastian

Hrm. We're basically relying on an (undocumented, I think?) implementation detail of smartlists. The way I noticed this was that I explicitly zeroed the unused parts of the smartlist whenever I removed an entry, which meant the old code broke. Afaict that's not illegal according to smartlist's contract but it breaks this code here.

comment:3 Changed 4 years ago by nickm

Keywords: TorCoreTeam201509 added

comment:4 Changed 4 years ago by Sebastian

Keywords: 026-backport added; TorCoreTeam201509 removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.7.x-final
Status: newneeds_review

As discussed on irc, published new branch with changes file and adding backports tag.

comment:5 Changed 4 years ago by nickm

Merged to master.

bbb73eaf31d3df5cde70ac4ca7145ffca053d53e is the thing to cherry-pick to 0.2.6 if we decide to do so.

comment:6 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.6.x-final

comment:7 Changed 4 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final
Resolution: fixed
Status: needs_reviewclosed

Not causing any actual trouble in 0.2.6, so far as anybody can tell. Not planning to backport. Let me know if I'm wrong.

Note: See TracTickets for help on using tickets.