1280578 - Wrap HeapAlloc/HeapFree

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect, P1)

Description

[Mike Hommey [:glandium]]

For rust code and for the few things in gecko that do use HeapAlloc/HeapFree, we need to wrap them so that they end up in jemalloc.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1280578 - Free pAdapterAddrs with GlobalFree in sctp_init_ifns_for_vrf().

Until da6f8ba in upstream git repository, sctp_init_ifns_for_vrf() was
using the MALLOC and FREE macros, using respectively HeapAlloc and
HeapFree. da6f8ba changed the allocations to use GlobalAlloc, but didn't
change the deallocations to use the symmetric GlobalFree.

This doesn't seem to cause direct problems, but when wrapping one family
of allocation functions, the dissymmetry causes allocator mismatch
problems.

Moreover, the MALLOC macro being unused, it might as well be removed,
along the FREE macro, so that both allocations and deallocations use one
API explicitly.

See https://github.com/sctplab/usrsctp/pull/92

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

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1280578 - Wrap HeapAlloc, HeapFree and HeapReAlloc on Windows.

The Win32 has non-malloc based heap allocation functions. They are not
widely used in the Gecko code base, but there are a few places that do
use them. They could be replaced with uses of malloc/free/realloc or
new/delete, but there is now an entirely separate problem that
requires wrapping those functions: the Rust static libraries are using
those functions to allocate memory.

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

Comment 3

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> Created attachment 8772335 [details]
> Bug 1280578 - Free pAdapterAddrs with GlobalFree in sctp_init_ifns_for_vrf().
> 
> Until da6f8ba in upstream git repository, sctp_init_ifns_for_vrf() was
> using the MALLOC and FREE macros, using respectively HeapAlloc and
> HeapFree. da6f8ba changed the allocations to use GlobalAlloc, but didn't
> change the deallocations to use the symmetric GlobalFree.
> 
> This doesn't seem to cause direct problems, but when wrapping one family
> of allocation functions, the dissymmetry causes allocator mismatch
> problems.
> 
> Moreover, the MALLOC macro being unused, it might as well be removed,
> along the FREE macro, so that both allocations and deallocations use one
> API explicitly.
> 
> See https://github.com/sctplab/usrsctp/pull/92
> 
> Review commit: https://reviewboard.mozilla.org/r/65170/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/65170/

Note this was merged upstream a few minutes ago. Also note that while looking at the history of the sctp code related to GlobalAlloc, I found a memory leak in sctp_userspace.c that happens to have been fixed a few months ago: https://github.com/sctplab/usrsctp/commit/36970fc3e8400dde4ea32a779488a14bfb9db7de

We may want to update sctp.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8772335 [details]
Bug 1280578 - Free pAdapterAddrs with GlobalFree in sctp_init_ifns_for_vrf().

https://reviewboard.mozilla.org/r/65170/#review62246

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Mike Hommey [:glandium] from comment #3)
> Note this was merged upstream a few minutes ago. Also note that while
> looking at the history of the sctp code related to GlobalAlloc, I found a
> memory leak in sctp_userspace.c that happens to have been fixed a few months
> ago:
> https://github.com/sctplab/usrsctp/commit/
> 36970fc3e8400dde4ea32a779488a14bfb9db7de
> 
> We may want to update sctp.

I'll ping Tuexen about what's a good point to do a new pull.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8772336 [details]
Bug 1280578 - Wrap HeapAlloc, HeapFree and HeapReAlloc on Windows.

https://reviewboard.mozilla.org/r/65172/#review62436

::: memory/mozalloc/winheap.cpp:53
(Diff revision 1)
> +LPVOID WINAPI HeapReAlloc(_In_ HANDLE hHeap, _In_ DWORD dwFlags,
> +                          _In_ LPVOID lpMem, _In_ SIZE_T dwBytes)
> +{
> +    if (dwFlags & (HEAP_REALLOC_IN_PLACE_ONLY | HEAP_ZERO_MEMORY)) {
> +        return NULL;
> +    }

So we basically reject requests using either of these flags, presumably because they're too hard? A brief comment explaining why would be useful.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0991c44fde
Free pAdapterAddrs with GlobalFree in sctp_init_ifns_for_vrf(). r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c0b92cae42
Wrap HeapAlloc, HeapFree and HeapReAlloc on Windows. r=njn

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c0991c44fde
https://hg.mozilla.org/mozilla-central/rev/87c0b92cae42