1448546 - clang builds causing mismatched operator new() and operator delete()

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Carl Corcoran [:ccorcoran]]

Using latest clang build, heap corruption may occur in mozglue in certain conditions. Minimal repro code is to add the following lines to the start of mozglue!DllBlocklist_Initialize:

> void *p = ::operator new(0x20);
> ::operator delete(p, 0x20);

This is the way some STL containers alloc/dealloc.

The first line will call into moz_xmalloc(), but the delete() call will call the CRT's free() function.

Looking at mozalloc.h, I believe we are missing the following two overloads of operator delete:

> MOZALLOC_EXPORT_NEW MOZ_ALWAYS_INLINE_EVEN_DEBUG
> void operator delete(void* ptr, size_t size) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
> {
>     return free_impl(ptr);
> }

> MOZALLOC_EXPORT_NEW MOZ_ALWAYS_INLINE_EVEN_DEBUG
> void operator delete[](void* ptr, size_t size) MOZALLOC_THROW_IF_HAS_EXCEPTIONS
> {
>     return free_impl(ptr);
> }

I'm going to continue investigating a bit...

Comment 1

[(Away)]

We used to have these sized dealloc functions, and we deleted them: https://hg.mozilla.org/integration/mozilla-inbound/rev/402cad93aa6e

Comment 2

[Carl Corcoran [:ccorcoran]]

clang-cl.exe is being passed -Zc:sizedDealloc- for this translation unit, but this doesn't prevent explicit calls to sized delete(), which the STL is doing.

If supplying these functions generates a warning (I'm not getting any such warnings locally), I'd think it's safer to provide them anyway and disable the warnings, as we know they can still be called. Thoughts?

Comment 3

[(Away)]

(In reply to Carl Corcoran [:ccorcoran] from comment #2)
> clang-cl.exe is being passed -Zc:sizedDealloc- for this translation unit,
> but this doesn't prevent explicit calls to sized delete(), which the STL is
> doing.
> 
> If supplying these functions generates a warning (I'm not getting any such
> warnings locally), I'd think it's safer to provide them anyway and disable
> the warnings, as we know they can still be called. Thoughts?

Redirecting...

Comment 4

[Nathan Froyd [:froydnj]]

I feel like the STL doing explicit calls to sized delete when the user has requested those calls to not be made is a bug.  What does the STL code look like for the explicit calls?

I think the warnings don't necessary come with clang-cl, they come with other compilers.  We could consider adding the functions just for Windows platforms, but I'd like to know if we could fix the problem upstream.  Is this a problem with MSVC builds as well? If it is a problem with MSVC, that sounds Very Not Good...

Comment 5

[Carl Corcoran [:ccorcoran]]

MSVC builds don't repro this; there even explicit calls to operator delete(void*,size_t) get transformed into non-sized delete.  In my local VS installs xmemory0 has this explicit call

> 	::operator delete(_Ptr, _Bytes);

Which is the default allocator used by std::string, std::vector, et al, and which invokes ::operator delete(_Ptr) when /Zc:sizedDealloc- is present for MSVC.

Clang however doesn't prevent these explicit calls, falling through to native HeapFree().

I do think the MSVC build should probably be addressed though; MSVC docs on this compiler switch: https://docs.microsoft.com/en-us/cpp/build/reference/zc-sizeddealloc-enable-global-sized-dealloc-functions

If I'm understanding the MSVC docs correctly, /Zc:sizedDealloc- doesn't prevent calls to sized delete, but rather prevents implicit calls to sized delete, no longer calling sized delete preferentially. It even says:

> Use the /Zc:sizedDealloc- option to preserve the old behavior, for example, when your code defines placement delete operators that use a second parameter of type size_t.

That sentence suggests that /Zc:sizedDealloc- still allows explicit calls to sized delete.

For that reason I think it's safest to provide the sized overloads, even in MSVC builds.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Carl Corcoran [:ccorcoran] from comment #5)
> MSVC builds don't repro this; there even explicit calls to operator
> delete(void*,size_t) get transformed into non-sized delete.  In my local VS
> installs xmemory0 has this explicit call
> 
> > 	::operator delete(_Ptr, _Bytes);
> 
> Which is the default allocator used by std::string, std::vector, et al, and
> which invokes ::operator delete(_Ptr) when /Zc:sizedDealloc- is present for
> MSVC.
> 
> Clang however doesn't prevent these explicit calls, falling through to
> native HeapFree().

OK, I think we can call this a clang-cl bug.  Do you want to file one or shall I?

> I do think the MSVC build should probably be addressed though; MSVC docs on
> this compiler switch:
> https://docs.microsoft.com/en-us/cpp/build/reference/zc-sizeddealloc-enable-
> global-sized-dealloc-functions
> 
> If I'm understanding the MSVC docs correctly, /Zc:sizedDealloc- doesn't
> prevent calls to sized delete, but rather prevents implicit calls to sized
> delete, no longer calling sized delete preferentially. It even says:
> 
> > Use the /Zc:sizedDealloc- option to preserve the old behavior, for example, when your code defines placement delete operators that use a second parameter of type size_t.
> 
> That sentence suggests that /Zc:sizedDealloc- still allows explicit calls to
> sized delete.
<
> For that reason I think it's safest to provide the sized overloads, even in
> MSVC builds.

The argument is that "explicit calls to sized delete can be made, therefore we should provide the necessary overloads," yes?  But I don't think that follows at all, because such calls are transformed to not call the sized delete overload, but rather the standard delete function.  That is, the overload we define will never get called (barring shenanigans like assembly making direct calls to the sized delete overload...).  I don't see how that's making things any safer...?

For people that define a sized delete-esque overload, such as:

http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.h#137

/Zc:sizedDealloc- ensures that we'll actually call their overload, rather than the global overload.  Am I understanding your position correctly?

Comment 7

[Carl Corcoran [:ccorcoran]]

Yep it seems MSVC is covered sufficiently with /Zc:sizedDealloc-.

Which clang-cl translates to -fno-sized-deallocation for which the docs are pretty light: 

> -fsized-deallocation, -fno-sized-deallocation
> Enable C++14 sized global deallocation functions

which points to a bug in clang-cl or clang.

What do you suggest? We could work around this by implementing the two overloads, wrapped in

> #if defined(__clang__) && defined(XP_WIN)

Comment 8

[Nathan Froyd [:froydnj]]

I would be OK with implementing the overloads solely for clang-cl, I think.

But...I'd also want to try and produce a standalone testcase that duplicates clang-cl's behavior here, so we can go to the clang-cl folks with a demonstrated disparity between what clang-cl does and what MSVC does.  Ideally we'd do this before we go adding the overloads, since the clang-cl folks tend to be reasonably fast with fixes, especially if there are testcases involved.  Do you want to try and produce a standalone testcase, or shall I?

Comment 9

[Nathan Froyd [:froydnj]]

Ah, I see that vcruntime_new.h unconditionally provides sized operator delete.  OK, maybe we should just go ahead and provide our own overload on Windows as well.

Comment 10

[Carl Corcoran [:ccorcoran]]

Attachment: Bug 1448546: Adding sized operator delete overloads on Windows builds;

Review commit: https://reviewboard.mozilla.org/r/231620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231620/

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8962771 [details]
Bug 1448546: Adding sized operator delete overloads on Windows builds;

https://reviewboard.mozilla.org/r/231620/#review237184

Thanks!  r=me with the change below.

::: memory/mozalloc/mozalloc.h:202
(Diff revision 1)
>  {
>      return free_impl(ptr);
>  }
>  
> +#if defined(XP_WIN)
> +MOZALLOC_EXPORT_NEW MOZ_ALWAYS_INLINE_EVEN_DEBUG

Can you add a comment here about providing these unconditionally because the Microsoft headers do, even though we compile without sized delete enabled?

Comment 12

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8962771 [details]
Bug 1448546: Adding sized operator delete overloads on Windows builds;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231620/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a6d3d03bb20
Adding sized operator delete overloads on Windows builds; r=froydnj

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7a6d3d03bb20