Opened 4 months ago

Closed 4 months ago

#22417 closed defect (fixed)

crash: double free or corruption (fasttop):

Reported by: toralf Owned by: Jigsaw52
Priority: High Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.2-alpha
Severity: Normal Keywords: tor-relay regression torrc reload crash
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

stable hardened Gentoo Linux server with 0.3.1.1_alpha2 while reloading config (splitted torrc into smaller files) :

and again I had to attach the trace isntead just putting it here into b/c TRAC rejected the input as being spam :-/

Child Tickets

Attachments (1)

t.tgz (10.0 KB) - added by toralf 4 months ago.
tar -cvf t.tgz torrc torrc.d/

Download all attachments as: .zip

Change History (21)

comment:2 Changed 4 months ago by toralf

Hhm, actually Tor does not crash but spews that if I reload the config unter Gentoo (kill -1 I do assume)

comment:3 Changed 4 months ago by toralf

And it happens, if an %include in torrc points to an non-existing directory and --verify-config is used

comment:4 Changed 4 months ago by toralf

mr-fox ~ # tor  --verify-config
May 27 16:27:29.867 [notice] Tor 0.3.1.2-alpha (git-61625b8f26384a4a) running on Linux with Libevent 2.1.8-stable, OpenSSL LibreSSL 2.5.4, Zlib 1.2.11, Liblzma 5.2.3, and Libzstd N/A.
May 27 16:27:29.867 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
May 27 16:27:29.867 [notice] This version is not a stable Tor release. Expect more bugs than usual.
May 27 16:27:29.867 [notice] Read configuration file "/etc/tor/torrc".

============================================================ T= 1495895249
Tor 0.3.1.2-alpha (git-61625b8f26384a4a) died: Caught signal 11
tor(+0x172296)[0x5594c9e62296]
tor(+0x178a1e)[0x5594c9e68a1e]
tor(+0x178a1e)[0x5594c9e68a1e]
tor(config_get_lines_include+0x38)[0x5594c9e68ec8]
tor(options_init_from_string+0x1df)[0x5594c9dedabf]
tor(options_init_from_torrc+0x1f1)[0x5594c9dedfd1]
tor(tor_init+0x313)[0x5594c9d3e1d3]
tor(tor_main+0x6e)[0x5594c9d3f67e]
tor(main+0x28)[0x5594c9d38c98]
/lib64/libc.so.6(__libc_start_main+0xfc)[0x7fc3f452e72c]
tor(_start+0x29)[0x5594c9d38ce9]
Aborted

comment:5 Changed 4 months ago by toralf

FWIW I have /etc/tor/torrc.d, within that afile which contains a single "%include..." line

comment:6 Changed 4 months ago by cypherpunks

Can you attach a minimal torrc which reproduces this issue when passed to tor with -f?

Changed 4 months ago by toralf

Attachment: t.tgz added

tar -cvf t.tgz torrc torrc.d/

comment:7 Changed 4 months ago by nickm

Keywords: tor-relay regression torrc reload crash added
Milestone: Tor: 0.3.1.x-final
Priority: MediumHigh

Thanks, Toralf! Does this also happen with 0.3.1.2-alpha? 0.3.1.1-alpha had a known double-free bug.

comment:8 Changed 4 months ago by toralf

oh yes, this is 0.3.1.2-alpha - sry for the typo in comment #0

comment:9 Changed 4 months ago by cypherpunks

I'm unable to reproduce the double free. Maybe someone else can try?

comment:10 Changed 4 months ago by arma

I also can't reproduce it, on Debian.

toralf, can you either give your explicit torrc files (are you running Tor as root like the # prompt implies) or better, run it under valgrind and give us a more useful trace?

Thanks!

comment:11 Changed 4 months ago by Jigsaw52

Status: newneeds_review

I was able to reproduce this and I fixed it. Here is the fix: https://github.com/Jigsaw52/tor/tree/fix-22417

The bug was triggered by including a folder with a non-empty file without any values followed by another file.

The problem is in the config_process_include() function. When this function is called to process an included folder, it will call config_get_included_list() for each file in the folder. config_get_included_list() will write the list of values on the file to included_list and the pointer to the last entry of the list to *list_last. When a file contains no values, both included_list and *list_last will be null. The bug is that, after calling config_get_included_list(), we update the the *next pointer with the following code:

    **next = included_list;
    *next = &(*list_last)->next;

When *list_last is null, we're are updating *next with the offset of next on the config_line_t struct (0x10 on a 64bit system). The next file on the folder will go through the same code and crash when writing to **next.

Last edited 4 months ago by Jigsaw52 (previous) (diff)

comment:12 in reply to:  11 Changed 4 months ago by arma

Replying to Jigsaw52:

The bug was triggered by including a folder with a non-empty file without any values followed by another file.

Agreed. For those trying to reproduce it, here's the trick:

mkdir newdir
echo " " > newdir/a
echo l > newdir/b
echo "%include newdir" > torrc
src/or/tor -f torrc

comment:13 Changed 4 months ago by arma

For those following along, the proposed fix is:

--- a/src/common/confline.c
+++ b/src/common/confline.c
@@ -300,7 +300,7 @@ config_process_include(const char *path, int recursion_level, int extended,
     tor_free(config_file);
 
     **next = included_list;
-    *next = &(*list_last)->next;
+    *next = !*list_last ? *next : &(*list_last)->next;
 
   } SMARTLIST_FOREACH_END(config_file);
   smartlist_free(config_files);

I can confirm that the change stops the crash.

But (not knowing the code here at all, though I admit I am stunned to find a ***next :) it is a confusing fix. If !*list_last, then assign *next to *next? If that's actually what you meant to do, wouldn't it be simpler to do it with something like

    if (*list_last)
      *next = &(*list_last)->next;

?

(I am in a twisty maze of pointers to pointers to pointers, all alike.)

comment:14 in reply to:  9 ; Changed 4 months ago by toralf

Replying to cypherpunks:

I'm unable to reproduce the double free. Maybe someone else can try?

Just for completeness:

The attached t.tgz does contain the non-empty no-vars-containing 60_* file. Shouldn't that help to reproduce the bug ?

comment:15 Changed 4 months ago by Jigsaw52

Assigning *next to *next is a no-op, so the code I wrote does the same as the version with the if.
However, the version with the if is indeed easier to read. I've changed it on my branch.

comment:16 in reply to:  14 Changed 4 months ago by cypherpunks

Replying to toralf:

Replying to cypherpunks:

I'm unable to reproduce the double free. Maybe someone else can try?

Just for completeness:

The attached t.tgz does contain the non-empty no-vars-containing 60_* file. Shouldn't that help to reproduce the bug ?

I extracted the tarball into a separate directory (not wanting to overwrite global configurations) without changing the %include. This caused Tor to abort prematurely because it couldn't find the /etc/tor/torrc.d/ directory and didn't reproduce the issue. The description in comment:3 lead me down the wrong path so in the end i wasn't able to reproduce it.

Changing the %include to the torrc.d directory in my extraction directory and the STR from comment:12 both reproduce the issue on my side now.

I agree with comment:13 that the 3-star (reference http://wiki.c2.com/?ThreeStarProgrammer) pointer is less than ideal.

comment:17 Changed 4 months ago by Jigsaw52

Here is a branch without the 3-star pointer: https://github.com/Jigsaw52/tor/tree/fix-22417-without-3-star

It replaces it with a 2-star pointer but duplicates the code to update the next pointer in config_get_lines_aux.

comment:18 Changed 4 months ago by Jigsaw52

Owner: set to Jigsaw52
Status: needs_reviewassigned

comment:19 Changed 4 months ago by Jigsaw52

Status: assignedneeds_review

comment:20 Changed 4 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged; added a changes file. Thanks for fixing!

Note: See TracTickets for help on using tickets.