839338 - ASan alloc/dealloc mismatch in _M_create_nodes/_M_destroy_nodes

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Christian Holler (:decoder)]

Using the test patch in bug 833127, I'm hitting another alloc/dealloc mismatch on mozilla-central (revision 91b9995f9ac7) when manually running the TestStartupCache test:

objdir-ff-asan64dbg$ LD_LIBRARY_PATH=dist/bin/ ./startupcache/test/TestStartupCache 2>&1 | asan_symbolize.py | c++filt
=================================================================
==3373== ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new vs free) on 0x7fddef237480
    #0 0x411552 in free ??:0
    #1 0x7fddfb2d80c9 in std::_Deque_base<base::AtExitManager::CallbackAndParam, std::allocator<base::AtExitManager::CallbackAndParam> >::_M_destroy_nodes(base::AtExitManager::CallbackAndParam**, base::AtExitManager::CallbackAndParam**) /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:634
    #2 0x7fddfb2d8020 in ~_Deque_base /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:557
    #3 0x7fddfb2d7f66 in ~deque /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:898
    #4 0x7fddfb2d6085 in AtExitManager /srv/repos/browser/mozilla-central/ipc/chromium/src/base/at_exit.cc:16
    #5 0x7fddfb1cab17 in NS_InitXPCOM2_P /srv/repos/browser/mozilla-central/xpcom/build/nsXPComInit.cpp:337
    #6 0x425358 in ScopedXPCOM /srv/repos/browser/mozilla-central/../../dist/include/testing/TestHarness.h:111
    #7 0x42088a in main /srv/repos/browser/mozilla-central/startupcache/test/TestStartupCache.cpp:490
    #8 0x7fddf657076c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
0x7fddef237480 is located 0 bytes inside of 512-byte region [0x7fddef237480,0x7fddef237680)
allocated by thread T0 here:
    #0 0x411a82 in operator new(unsigned long) ??:0
    #1 0x7fddfb2d7e41 in std::_Deque_base<base::AtExitManager::CallbackAndParam, std::allocator<base::AtExitManager::CallbackAndParam> >::_M_create_nodes(base::AtExitManager::CallbackAndParam**, base::AtExitManager::CallbackAndParam**) /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:619
    #2 0x7fddfb2d7947 in std::_Deque_base<base::AtExitManager::CallbackAndParam, std::allocator<base::AtExitManager::CallbackAndParam> >::_M_initialize_map(unsigned long) /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:593
    #3 0x7fddfb2d7729 in _Deque_base /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_deque.h:470
    #4 0x7fddfb2d607d in AtExitManager /srv/repos/browser/mozilla-central/ipc/chromium/src/base/at_exit.cc:16
    #5 0x7fddfb1cab17 in NS_InitXPCOM2_P /srv/repos/browser/mozilla-central/xpcom/build/nsXPComInit.cpp:337
    #6 0x425358 in ScopedXPCOM /srv/repos/browser/mozilla-central/../../dist/include/testing/TestHarness.h:111
    #7 0x42088a in main /srv/repos/browser/mozilla-central/startupcache/test/TestStartupCache.cpp:490
    #8 0x7fddf657076c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
==3373== HINT: if you don't care about these warnings you may set ASAN_OPTIONS=alloc_dealloc_mismatch=0
Stats: 0M malloced (0M for red zones) by 101 calls
Stats: 0M realloced by 0 calls
Stats: 0M freed by 2 calls
Stats: 0M really freed by 0 calls
Stats: 24M (24M-0M) mmaped; 6 maps, 0 unmaps
  mmaps   by size class: 8:16383; 9:8191; 10:4095; 11:2047; 12:1024; 16:64;
  mallocs by size class: 8:58; 9:29; 10:3; 11:5; 12:5; 16:1;
  frees   by size class: 8:1; 10:1;
  rfrees  by size class:
Stats: malloc large: 0 small slow: 6
Stats: StackDepot: 0 ids; 0M mapped
==3373== ABORTING

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: testcase

I forgot to also mark the operator new that don't take a mozilla::fallible_t in the previous patch. Can you try this one?

Comment 2

[Christian Holler (:decoder)]

Comment on attachment 711786 [details] [diff] [review]
testcase

Confirmed, this is working \o/

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Another way to do it

Looks like mozalloc.h was already trying to use always_inline, but failing for two reasons:

* Not including the correct mfbt file
* MFBT not defining MOZ_ALWAYS_INLINE to something that forces inlining in debug builds :-(

Let me know if this fixes the bug for you and if try is also happy I will add a big explanation and ask Jeff Walden for review.

Comment 4

[Christian Holler (:decoder)]

Comment on attachment 712476 [details] [diff] [review]
Another way to do it

Also seems to work :) I also upgraded Clang in the middle though, I hope that didn't influence this testing.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 712476 [details] [diff] [review]
Another way to do it

A try push is at

https://tbpl.mozilla.org/?tree=Try&rev=ba52a8c8fa3b

The change to mozalloc.h should be clear, without including mozilla/Attributes.h we depend on the include order in other files to get MOZ_ALWAYS_INLINE or not.

I could not find why we currently have

#  define MOZ_ALWAYS_INLINE     MOZ_INLINE

but this bug has a case where we do want to inline, even on debug. Another option would be to have a MOZ_REALLY_ALWAYS_INLINE, but if we don't need one that is better.

As to why we want to inline even on debug builds:

We expect to be able to replace all variants of operator new and delete. The problem is that some of them are forward declared in system headers and get explicitly marked as having default visibility.

Having default visibility, if they are not inlined it is up to the linker if we end up using ours or the one provided by libstdc++.

We found this with asan because one version of new has a mozilla type (mozilla::fallible_t) and gets hidden linkage. The operator delete is generic and is forward declared in a system header with default visibility. The net result is that we can our new and libstdc++'s delete.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 712476 [details] [diff] [review]
Another way to do it

Review of attachment 712476 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Attributes.h
@@ -33,5 @@
>   * compilers are not guaranteed to respect it (although they're much more likely
>   * to do so).
>   */
> -#if defined(DEBUG)
> -#  define MOZ_ALWAYS_INLINE     MOZ_INLINE

...

Much agreed about not having MOZ_ALWAYS_INLINE_FOR_REALS_YO and doing this instead.  I have no idea why this was ever this way.  I guess we'll find out when we land this!

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1bef2ab93e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e1bef2ab93e