allocator shim refactoring subtly broke the symbol aliasing logic on Linux |
|||
Issue descriptionFollows up from pcc@ findings on https://codereview.chromium.org/2720703004/ Looks like https://codereview.chromium.org/2697123007/ caused a very subtle bug that wasn't detected neither by the compiler (in the default config) neither the reviewer (me). The problem is the following: That CL did change the signature of ShimMalloc and friends adding a void* context pointer argument, i.e. from: void* ShimMalloc(size_t size) to void* ShimMalloc(size_t size, void* context) In turn, on Linux, we were using symbol aliasing to avoid one indirection layer (form malloc -> ShimMalloc) as follows: SHIM_ALWAYS_EXPORT void* malloc(size_t size) __THROW SHIM_ALIAS_SYMBOL(ShimMalloc); Unfortunately, now the signatures of the two don't match anymore. This essentially works now (maybe) by pure luck, just because of the way the x86 and x86_64 ABI works (and because we effectively ignore the |context| on Linux), however it is extremely very fragile (read: we should not ship chrome in this state) and also seem to break some internal bot using -fsanitize=cfi-icall . pcc@ is proposing a change which essentially removes the alias and uses a function call. However that means introducing an extra indirection layer which makes me a bit nervous performance wise. erikchen@: would be great if we could come up with a way to avoid this. I was thinking something along the lines of: #if OSX #define OPTIONAL_CONTEXT_ARG , void* context #define PASS_OPTIONAL_CONTEXT_ARG , nullptr #else #define OPTIONAL_CONTEXT_ARG #define PASS_OPTIONAL_CONTEXT_ARG #endif or alternatively #define OPTIONAL_CONTEXT_ARG(x) x and then return chain_head->alloc_function(chain_head, size OPTIONAL_SHIM_CONTEXT(, nullptr)); WDYT? In any case should solve this asap.
,
Feb 28 2017
> On my machine at least all of the shim functions except for ShimValloc and ShimRealloc are inlined into the interceptor functions. Ah interesting. I didn't realize that the override_ headers are included directly after allocator_shim.cc so they can actually be inlined by the compiler without relying on linker LTO or such. If we could guarantee that they won't regress at random point (does always_inline do what it says? I stopped trusting the fact that compilers do what I ask a long time ago :P) I don't care at all about ShimValloc honestly, it's quite rare. ShimRealloc, instad, conversely to what one might think it's used very frequently. There are large portions of the codebase that essentially use realloc() as a malloc() (skia was a big one IIRC).
,
Feb 28 2017
> If we could guarantee that they won't regress at random point (does always_inline do what it says? I stopped trusting the fact that compilers do what I ask a long time ago :P) That is at least how it is documented (https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html says "For functions declared inline, this attribute inlines the function independent of any restrictions that otherwise apply to inlining."). It doesn't seem too unreasonable to rely on that documentation. Although I noticed that it isn't always possible to define an alias to an always_inline function (because the definition is not necessarily emitted). So for example if I apply ALWAYS_INLINE to ShimCppNew I get an error when compiling with GCC: ../../base/allocator/allocator_shim_override_cpp_symbols.h:33:26: error: ‘void* operator new [](size_t, const std::nothrow_t&)’ aliased to external symbol ‘ShimCppNew’ So we would need to replace all aliases to shim functions with real function calls. That doesn't seem too bad though because it will prevent bugs like this happening in future, and it seems like there are other instances of the same bug (e.g. in base/allocator/allocator_shim_override_linker_wrapped_symbols.h).
,
Feb 28 2017
Oh yes they can't be alias at that point . Which is fine, if we get the shim call inlined the final number of hopping layers should be the same. So, as long as we can force inline, I am up for dropping the alias and have actual implementations that call into the inline shim functions. (Also I have seen the comment on keeping the alias for__libc functions. Alright there as long as we have a comment explaining why. Honestly I wouldn't care if those don't get inlined as their use should be extremely rare. But from what I recall you said it might convince the compiler to not inline) If somebody can file a patch there I can take a look later.
,
Feb 28 2017
Okay, I'll change my patch to use ALWAYS_INLINE and drop all uses of aliases. If ALWAYS_INLINE is on the shim functions they should be inlined everywhere including the duplicate bodies in the __libc functions. I also realised that there should be no downside to doing that: because we use ICF the linker should be able to merge the __libc functions with the non-__libc functions.
,
Feb 28 2017
pcc: Thanks for dealing with this issue. It sounds like we won't have an issue after your patch, so assigning this to you.
,
Mar 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d4809ef025d50acb50489134273e13444252a8e6 commit d4809ef025d50acb50489134273e13444252a8e6 Author: pcc <pcc@chromium.org> Date: Wed Mar 01 11:01:30 2017 allocator: Fix function type mismatch in allocator function definitions. CL 2697123007 added a context argument to most of the shim allocator functions. This means that it is no longer valid to define (say) malloc as an alias of ShimMalloc, because their function types no longer match. This change defines the allocator functions as real functions that call the shims passing nullptr as the context. Found with -fsanitize=cfi-icall. R=primiano@chromium.org BUG= 696986 , 601497 Review-Url: https://codereview.chromium.org/2720703004 Cr-Commit-Position: refs/heads/master@{#453900} [modify] https://crrev.com/d4809ef025d50acb50489134273e13444252a8e6/base/allocator/allocator_shim.cc [modify] https://crrev.com/d4809ef025d50acb50489134273e13444252a8e6/base/allocator/allocator_shim_internals.h [modify] https://crrev.com/d4809ef025d50acb50489134273e13444252a8e6/base/allocator/allocator_shim_override_cpp_symbols.h [modify] https://crrev.com/d4809ef025d50acb50489134273e13444252a8e6/base/allocator/allocator_shim_override_glibc_weak_symbols.h [modify] https://crrev.com/d4809ef025d50acb50489134273e13444252a8e6/base/allocator/allocator_shim_override_libc_symbols.h [modify] https://crrev.com/d4809ef025d50acb50489134273e13444252a8e6/base/allocator/allocator_shim_override_linker_wrapped_symbols.h
,
Mar 1 2017
,
Mar 1 2017
Issue 694441 has been merged into this issue. |
|||
►
Sign in to add a comment |
|||
Comment 1 by p...@chromium.org
, Feb 28 2017