Opened 2 years ago

Closed 23 months ago

#17218 closed enhancement (implemented)

Move most of "circuit_mark_for_close" into "circuit_free" or "circuit_close_all_marked".

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Major Keywords:
Cc: Actual Points:
Parent ID: #16764 Points:
Reviewer: Sponsor: SponsorS

Description

The function "circuit_mark_for_close()" calls a few other functions (notably circuit_build_failed() and rend_client_report_intro_point_failure()) that call into the blob. If we pull it out, we would chop the remaining blob in half.

We should use a system similar to the one we use for closeable connections where we put closeable circuits on a list, rather than iterating over the entire circuit list.

Doing this would remove more than half of the functions currently in the blob as currently measured. (!)

Child Tickets

Attachments (1)

0001-Fix-memory-leak-by-circuit-marked-for-close-list.patch (843 bytes) - added by cypherpunks 23 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 2 years ago by nickm

my branch decouple_circuit_mark tries to do this.

comment:2 Changed 2 years ago by nickm

Status: newneeds_review

comment:3 Changed 2 years ago by dgoulet

Severity: Normal

This is currently running on my fast relay, we'll see in the coming days if any issues arise.

With master at commit 19e10f95c105a698d3593a1ecfa88a7449a21127, when sending SIGTERM to tor, here is a backtrace of an assertion:

Oct 27 14:23:33.000 [notice] Catching signal TERM, exiting cleanly.
Oct 27 14:23:33.000 [warn] Couldn't find circuit 4107801626 (for channel 1232)
Oct 27 14:23:33.000 [err] tor_assertion_failed_(): Bug: src/or/circuitmux.c:501: circuitmux_detach_all_circuits: Assertion to_remove->muxinfo.policy_data == NULL failed; aborting. (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug: Assertion to_remove->muxinfo.policy_data == NULL failed in circuitmux_detach_all_circuits at src/or/circuitmux.c:501. Stack trace: (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(log_backtrace+0x41) [0x7f4d6c206981] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(tor_assertion_failed_+0x94) [0x7f4d6c2145b4] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(circuitmux_detach_all_circuits+0x2a1) [0x7f4d6c195c91] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(+0xa5cae) [0x7f4d6c182cae] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(channel_free_all+0x8f) [0x7f4d6c18319f] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(tor_free_all+0x7a) [0x7f4d6c11937a] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(tor_cleanup+0x2c) [0x7f4d6c1194ec] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(+0x3d664) [0x7f4d6c11a664] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     /usr/lib/x86_64-linux-gnu/libevent-2.0.so.5(event_base_loop+0xa7b) [0x7f4d6b76324b] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(do_main_loop+0x20d) [0x7f4d6c11884d] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(tor_main+0x19ad) [0x7f4d6c11c96d] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(main+0x19) [0x7f4d6c115ba9] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7f4d6ab59ec5] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)
Oct 27 14:23:33.000 [err] Bug:     ./git/tor/src/or/tor(+0x38bfb) [0x7f4d6c115bfb] (on Tor 0.2.8.0-alpha-dev 81b6f18dbef9187b)

comment:4 Changed 2 years ago by dgoulet

I just got this (couple minutes after start):

Oct 27 14:26:26.Oct 27 14:26:26.000 [warn] Remote server sent bogus reason code 65023

comment:5 Changed 2 years ago by nickm

dgoulet has still been testing this, and hasn't seen anything weird. That's pretty neat!

So at this point, I'm fairly confident this won't kill relays. I wonder if it will be bad for clients or HS services.

comment:6 Changed 2 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Tried it for a while, and it seemed to work for me. It passed all the tests, including test-stem and test-network-all. It's merging time!

comment:7 Changed 2 years ago by nickm

The blob is now down to 38 functions!!

comment:8 Changed 2 years ago by dgoulet

Resolution: implemented
Severity: NormalMajor
Status: closedreopened

comment:3 is still happening every time tor stops.

comment:9 Changed 2 years ago by nickm

(Could we have a new ticket for that one? This ticket actually worked.)

comment:10 Changed 23 months ago by cypherpunks

The commit implementing this ticket (8b4e5b7ee902fb7fa07767410a18433d752c7aef) introduces a memory leak by not freeing the memory held by the new circuit list when tor tries to free its resources. The attached patch fixes the issue.

comment:11 Changed 23 months ago by nickm

Status: reopenedneeds_review

comment:12 Changed 23 months ago by teor

This is a simple patch to fix a memory leak, let's get it merged.

comment:13 Changed 23 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged!

Note: See TracTickets for help on using tickets.