Remove --enable-openbsd-malloc (Tor maxes CPU when --enable-openbsd-malloc is used)

When tor is built with --enable-openbsd-malloc running 'make test' consumes 100% of the CPU. Attaching a debugger to the process shows it looping forever in calloc (src/ext/OpenBSD_malloc_Linux.c). Changing from -O2 to -O3 (or -O1) in the Makefile "fixes" (as in the tests run) the issues so it seems to be a compiler bug.

Some more details: Debian testing, x86_64

$ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/6/lto-wrapper Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Debian 6.2.0-6' --with-bugurl=file:///usr/share/doc/gcc-6/README.Bugs --enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-6 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-6-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-6-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-6-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 6.2.0 20161010 (Debian 6.2.0-6)

I've attached the output of objdump for the failing and working object files, but I'm too stupid to debug it any further, so hopefully someone more intelligent can.

Trac:
Username: icanhasaccount

changed milestone to %Tor: 0.3.5.x-final

added 034-triage-20180328 035-triaged-in-20180711 component::core tor/tor fast-fix milestone::Tor: 0.3.5.x-final points::.2 priority::low reporter::icanhasaccount resolution::fixed review-group-31 reviewer::ahf severity::minor status::closed type::defect labels

Trac:
Username: icanhasaccount

O1.txt

O1 - make test passes

Trac:
Username: icanhasaccount

Trac:
Username: icanhasaccount

O2.txt

O2 - make test fails

Trac:
Username: icanhasaccount

I can confirm this (reproducing it on Fedora with gcc-6.2.1-2.fc24.x86_64). This definitely looks like a compiler bug to me.

Trac:
Milestone: N/A to Tor: 0.3.0.x-final

This bug seems to depend on the name of the "malloc" function. I'm attaching a minimal example to reproduce this bug (down to 67 lines from 2049). The comment near the head of the file shows three ways to build this: two of which work, and one of which gives us the infinite loop that you ran into.

I think it's time to report this upstream to gcc, unless they already know about it.

Trac:
example.c

Trac:
example.2.c

I've attached an even simpler example.

Yep - example.2.c shows the same problem. I'll report it to the gcc folk if its not reported already.

Thanks!

Trac:
Username: icanhasaccount

Trac:
example2.c

Variant example, with no headers included.

I think they might already know: here's a bug report that looks quite similar.

https://gcc.gnu.org/ml/gcc-help/2015-03/msg00091.html

The -fno-builtin-malloc option seems to be a workaround here. In production, you might need -fno-builtin-... for all of malloc, free, realloc, etc.

I wonder if it would be worth removing the option to build with openbsd malloc from future releases?

If the email relates to the same issue its been broken for a while now (the email is from 2015 and gcc 5) and no one else has run into it.

Trac:
Username: icanhasaccount

FWIW the OpenBSD memory allocator was added in ticket:468#comment:22.

I agree with comment:8 with regards to removing the openbsd malloc build option. It's obvious the code isn't tested or maintained and causes people to shoot themselves in the foot.

Trac:
Username: icanhasaccount

remove_openbsd_malloc.diff

Diff to remove openbsd malloc if thats desirable

Trac:
Username: icanhasaccount

I wonder if it would be worth removing the option to build with openbsd malloc from future releases?

A while ago, lots and lots of relays were built to use it, because of glibc malloc problems with memory fragmentation. Have those all gone away?

Sorry - I didn't look too closely at why the openbsd allocator was added in the first place. That was ignorant and lazy of me.

Adding the following to configure.ac (as suggested by Nick in comment 7) allows the tests to run. I'm not familiar with autotools though so perhaps there would be a cleaner way to do this if we could narrow it down to a specific version of gcc?

Trac:
Username: icanhasaccount

Trac:
Username: icanhasaccount

configure_ac_patch.patch

Work around compilation issues with some versions of gcc

Trac:
Username: icanhasaccount

Renaming ticket, as my understanding is that this is not test-specific, and it occurs with tor in general when the --enable-openbsd-malloc feature is enabled.

Trac:
Summary: Make test fails when --enable-openbsd-malloc is used to Tor maxes CPU when --enable-openbsd-malloc is used

Triaged out on December 2016 from 030 to 031.

Trac:
Keywords: N/A deleted, triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-final to Tor: 0.3.1.x-final

Trac:
Points: N/A to .2
Status: new to needs_revision

0.3.1 feature freeze today: these don't seem like they will be all sorted out in time. Let's try to make progress in 0.3.2.

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.2.x-final

Trac:
Keywords: triage-out-030-201612 deleted, N/A added

Trac:
Summary: Tor maxes CPU when --enable-openbsd-malloc is used to Remove --enable-openbsd-malloc (Tor maxes CPU when --enable-openbsd-malloc is used)
Milestone: Tor: 0.3.2.x-final to Tor: 0.3.4.x-final

gcc 7 simply deletes the functions, so --enable-openbsd-malloc now instead does nothing except slightly increase compilation resources and executable size.

proposal: remove tcmalloc and openbsd malloc, add jemalloc and enable by default on Linux if installed.

pros compared to ptmalloc2:

• moderately decreased CPU usage (#23777 (moved))
• slightly decreased memory usage (fragmentation is much less bad in 0.3.2 already, but jemalloc still does better by default)

pros compared to openbsd malloc:

• remove dead code
• code path will be actually tested (unlike with openbsd malloc)
• jemalloc is actually maintained by distributions and -ljemalloc is a standard method of overriding glibc. this way will continue to work, unlike just defining functions

cons:

• if always-on, adds an extra prerequisite. if used only when available, might cause more Linux-only bugs
• glibc does slightly more runtime checks, but AFAIK these are almost entirely useless for security
• might be fingerprintable? seems highly unlikely, and would also apply to other OSes

Trac:
Cc: N/A to alex_y_xu@yahoo.ca

Trac:
0001-Add-with-jemalloc.-20424-23777.patch

patch to add --with-jemalloc

so I read what I'm pretty sure is all of the information online about replacing malloc, and I'm pretty sure that this is the best global option possible. another option that can be explored is to have tor_malloc_ call jemalloc, but AFAICT most distros build jemalloc with no prefix, so dlopen magic would be needed.

Trac:
Status: needs_revision to needs_review

Trac:
0001-Add-with-malloc-configure-option.-20424-23777.patch

so apparently autoconf calls it LIBS

Trac:
Keywords: N/A deleted, review-group-31 added

Trac:
Reviewer: N/A to nickm

(I'm guessing that the second patch should be applied without the first, right?)

I like this patch, but I'm not so sure about having jemalloc be the default right away. Let's let people test it as an option before we decide it's the best choice for everyone. Also, if I'm reading the patch right, this code will break on Linux if jemalloc isn't installed, which probably isn't what you'd intended?

Other questions:

• Why move the mlockall check?
• Why does the patch move dmalloc? Have you tested this? Does it work?
• Where and how have you tested this patch?

Other notes:

• This will need a changes file, explaining to people how to use this feature.
• We'll still want to remove the openbsd_malloc code.

Trac:
Status: needs_review to needs_revision

I wanted to put all of the allocator-related configure.ac code together. I tested at least one of the versions of the patches both with and without jemalloc installed, but I only tested on Linux. The plan was to remove openbsd malloc first. I see no benefit to using it over jemalloc.

I still need to test what happens when rust is linked in.

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

Trac:
Keywords: 034-removed-20180328 deleted, fast-fix added

Move most needs_revision tickets from 0.3.4 to 0.3.5: we're about to hit the feature freeze.

If you really need to get this into 0.3.4, please drop me a line. :)

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final

I'd like to point out that jemalloc has more issues with security than either ptmalloc or OpenBSD's malloc. The large, aligned heaps make ASLR less effective (and that's one reason why Firefox on *nix machines is so much easier to compromise than on Windows).

I asked Firefox people and they said that Firefox uses "jemalloc for Windows/Mac/Linux/Android, system malloc on iOS".

additionally, starting with 5, jemalloc now uses extent based allocation, so from what I understand, ASLR will be just as good as with other allocators.

They use jemalloc for Windows? I was under the impression Windows used the system malloc. And yeah I know newer versions of jemalloc are improved (redzones, etc), but I'm still a little weary, especially given that OpenBSD's malloc is actually really well-designed from a security standpoint. Though honestly the Copperhead malloc (a hybrid of bionic and OpenBSD's) is even better, but I feel it's unlikely that it would be included in Tor.

Trac:
Keywords: N/A deleted, 035-triaged-in-20180711 added

Let's try to get this unstuck, by disentangling the various options.

I've made a branch remove_openbsd_malloc that ports icanhasaccount's removal code to master if we decide to do that. PR at https://github.com/torproject/tor/pull/224 . Not my preferred approach.

And I've made a branch called with_malloc that ports the parts of Hello71's patch that I agree with to master: it adds --with-malloc=, doesn't change our default, doesn't break old configure options, and keeps openbsd malloc as an option. Works for me, but probably needs more review and testing. PR at https://github.com/torproject/tor/pull/225 .

The above two branches apply to master only and are mutually exclusive, though we could probably combine them into one.

For backport purposes, I have a branch fix_nonstandard_malloc_029 that only does the minimum needed to fix the originally supported issue. PR at https://github.com/torproject/tor/pull/226 . Works for me.

Trac:
Status: needs_revision to needs_review
Reviewer: nickm to N/A

Replying to nickm:

Let's try to get this unstuck, by disentangling the various options.

I've made a branch remove_openbsd_malloc that ports icanhasaccount's removal code to master if we decide to do that. PR at https://github.com/torproject/tor/pull/224 . Not my preferred approach.

And I've made a branch called with_malloc that ports the parts of Hello71's patch that I agree with to master: it adds --with-malloc=, doesn't change our default, doesn't break old configure options, and keeps openbsd malloc as an option. Works for me, but probably needs more review and testing. PR at https://github.com/torproject/tor/pull/225 .

The above two branches apply to master only and are mutually exclusive, though we could probably combine them into one.

For backport purposes, I have a branch fix_nonstandard_malloc_029 that only does the minimum needed to fix the originally supported issue. PR at https://github.com/torproject/tor/pull/226 . Works for me.

Thanks for fixing my patch and giving credit, but I'd appreciate it if you called me Alex Xu in changelog, or at least not put my nick in quotation marks (looks like scare quotes to me). Also, I think you put too many twos in the change file.

On the patch itself, I did actually test what happens when Rust is linked in: it works fine. I think Rust probably uses the system allocator on Linux, so replacing the malloc this way in glibc should also replace the Rust allocator and everything works fine.

Also, I'm not sure it's a good idea to leave all of these options in if nobody is actually going to test them (as shown by openbsd malloc being broken by all this time and nobody really caring that much to fix it).

ok, added a squash commit to fix ticket number and credit you in the form you prefer.

(FYI, our house style has been to put pseudonyms that don't look like names into quotation marks, since not doing so has caused confusion in the past with strings like "patch from randomguessing" or "patch from Some Guy".)

OK, thanks!

Trac:
Reviewer: N/A to ahf

• https://github.com/torproject/tor/pull/224 - looks good to me. The Travis failure looks like it was an issue that was already fixed.
• https://github.com/torproject/tor/pull/225 - looks good.
• https://github.com/torproject/tor/pull/226 - looks good.

Trac:
Status: needs_review to merge_ready

I've merged fix_nonstandard_malloc_029 to 0.2.9 and forward.

I've squashed with_malloc and merged it to master, then added a commit in 5597ddc36047a4 to mark openbsd-malloc as deprecated.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

changed time estimate to 1h 36m

mentioned in issue #23777 (moved)

mentioned in issue #26093 (moved)

mentioned in issue #26594 (moved)

mentioned in issue #27037 (moved)

moved to tpo/core/tor#20424 (closed)

mentioned in issue tpo/core/tor#27037 (closed)