#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 20 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 22 months ago by nickm

my branch decouple_circuit_mark tries to do this.

comment:2 Changed 22 months ago by nickm

  • Status changed from new to needs_review

comment:3 Changed 21 months ago by dgoulet

  • Severity set to 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 21 months 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 21 months 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 21 months ago by nickm

  • Resolution set to implemented
  • Status changed from needs_review to closed

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 21 months ago by nickm

The blob is now down to 38 functions!!

comment:8 Changed 20 months ago by dgoulet

  • Resolution implemented deleted
  • Severity changed from Normal to Major
  • Status changed from closed to reopened

comment:3 is still happening every time tor stops.

comment:9 Changed 20 months ago by nickm

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

comment:10 Changed 20 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 20 months ago by nickm

  • Status changed from reopened to needs_review

comment:12 Changed 20 months ago by teor

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

comment:13 Changed 20 months ago by nickm

  • Resolution set to implemented
  • Status changed from needs_review to closed

Merged!

Note: See TracTickets for help on using tickets.