Closed Bug 1280578 Opened 3 years ago Closed 3 years ago

Wrap HeapAlloc/HeapFree

Categories

(Core :: Memory Allocator, defect, P1)

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files)

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.
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/
Attachment #8772335 - Flags: review?(rjesup)
Attachment #8772336 - Flags: review?(n.nethercote)
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/
(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 on attachment 8772335 [details]
Bug 1280578 - Free pAdapterAddrs with GlobalFree in sctp_init_ifns_for_vrf().

https://reviewboard.mozilla.org/r/65170/#review62246
Attachment #8772335 - Flags: review?(rjesup) → review+
(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 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.
Attachment #8772336 - Flags: review?(n.nethercote) → review+
Pushed by mh@glandium.org:
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
https://hg.mozilla.org/mozilla-central/rev/8c0991c44fde
https://hg.mozilla.org/mozilla-central/rev/87c0b92cae42
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.