Opened 4 months ago

Closed 4 months ago

#30629 closed defect (fixed)

We seem to be reading some freed events on exit

Reported by: arma Owned by: nickm
Priority: High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.4.1.1-alpha
Severity: Major Keywords: 041-must, tor-events, regression, memory-safety, 041-regression, valgrind, dgoulet-merge
Cc: nickm Actual Points: .1
Parent ID: Points: 1
Reviewer: ahf Sponsor: Sponsor31-can

Description

Run your Tor master as a client under valgrind:

$ valgrind --leak-check=full src/app/tor

and wait for it to bootstrap to 100%. Then ctrl-c it.

On exit, valgrind will give you a pile of complaints like

==4119== Invalid read of size 8
==4119==    at 0x4C1DB9C: ??? (in /usr/lib/x86_64-linux-gnu/libevent-2.1.so.6.0.2)
==4119==    by 0x4C21A78: event_free (in /usr/lib/x86_64-linux-gnu/libevent-2.1.so.6.0.2)
==4119==    by 0x2ADA19: tor_event_free_ (compat_libevent.c:76)
==4119==    by 0x2ADA19: mainloop_event_free_ (compat_libevent.c:461)
==4119==    by 0x17748B: tor_mainloop_free_all (mainloop.c:2523)
==4119==    by 0x1665FB: subsystems_shutdown_downto (subsysmgr.c:185)
==4119==    by 0x165FB4: tor_free_all (shutdown.c:162)
==4119==    by 0x164B54: tor_run_main (main.c:1360)
==4119==    by 0x1620F9: tor_main (tor_api.c:164)
==4119==    by 0x161CB8: main (tor_main.c:32)
==4119==  Address 0x5489ec0 is 432 bytes inside a block of size 664 free'd
==4119==    at 0x48369AB: free (vg_replace_malloc.c:530)
==4119==    by 0x2ADB20: tor_libevent_free_all (compat_libevent.c:490)
==4119==    by 0x165FAF: tor_free_all (shutdown.c:160)
==4119==    by 0x164B54: tor_run_main (main.c:1360)
==4119==    by 0x1620F9: tor_main (tor_api.c:164)
==4119==    by 0x161CB8: main (tor_main.c:32)
==4119==  Block was alloc'd at
==4119==    at 0x483577F: malloc (vg_replace_malloc.c:299)
==4119==    by 0x310F47: tor_malloc_ (malloc.c:45)
==4119==    by 0x4C1E9B3: event_mm_calloc_ (in /usr/lib/x86_64-linux-gnu/libevent-2.1.so.6.0.2)
==4119==    by 0x4C224D9: event_base_new_with_config (in /usr/lib/x86_64-linux-gnu/libevent-2.1.so.6.0.2)
==4119==    by 0x2AD284: tor_libevent_initialize (compat_libevent.c:158)
==4119==    by 0x28E879: init_libevent (config.c:8031)
==4119==    by 0x28E879: options_act_reversible (config.c:1466)
==4119==    by 0x28E879: set_options (config.c:934)
==4119==    by 0x290721: options_init_from_string (config.c:5529)
==4119==    by 0x290CA9: options_init_from_torrc (config.c:5293)
==4119==    by 0x1632A6: tor_init (main.c:619)
==4119==    by 0x163B13: tor_run_main (main.c:1297)
==4119==    by 0x1620F9: tor_main (tor_api.c:164)
==4119==    by 0x161CB8: main (tor_main.c:32)

maint-0.4.0 does not have this bug, and tor-0.4.1.1-alpha does.

A git bisect brought me to commit 6eb1b8da0ab2, which is about periodic events so it looks promising. It's from #30293.

Child Tickets

Change History (9)

comment:1 Changed 4 months ago by teor

Cc: nickm dgoulet added
Keywords: 041-must tor-events regression memory-safety added
Points: 1
Priority: MediumHigh
Severity: NormalMajor
Sponsor: Sponsor31-can

This is a memory safety bug, so we must fix it before the 0.4.1 stable release.

It was caused by code for sponsor 31, so we can fix it under that sponsor.

comment:2 Changed 4 months ago by nickm

Keywords: 041-regression valgrind added

comment:3 Changed 4 months ago by nickm

Owner: set to nickm
Status: newaccepted

I'm tentatively assigning this to myself, but please let me know if you can take it instead, David?

comment:4 Changed 4 months ago by nickm

The problem here appears to be that we are now calling tor_libevent_free_all earlier than we did before. I think I have (famous last words!) an easy solution.

comment:5 Changed 4 months ago by nickm

Actual Points: .1
Status: acceptedneeds_review

Took a while to figure it out, but the fix was indeed trivial. We will have a better fix in 0.4.2 as part of our s31 modularization work.

See branch ticket30629 with PR at https://github.com/torproject/tor/pull/1057 . It WFM but let's see what the CI says.

comment:6 Changed 4 months ago by ahf

Status: needs_reviewmerge_ready

Looks good.

comment:7 Changed 4 months ago by nickm

Keywords: dgoulet-merge added

comment:8 Changed 4 months ago by dgoulet

Cc: dgoulet removed
Reviewer: ahf

Merged!

comment:9 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.