Signed-off-by: Michael Stefaniuc mstefani@winehq.org --- I know that Alexandre and Jacek wanted to work on the HeapAlloc wrappers. But I don't remember if I had promised (or not) at the last WineConf to dig up my old patch and polish it up and submit it to serve as a discussion start point. Anyway, here it is with some explanations.
The _nofail variants should not "fail" memory allocations and will raise an exception on out of memory conditions. heap_alloc_nofail thus will never return NULL. heap_realloc_nofail can still return NULL but *only* when the alloc size is 0 and thus functions as heap_free. Checking the returned pointer for NULL is not needed when the size != 0. And the "assign to temp pointer to prevent mem leak on failure" workaround is never needed.
The _nofail variants should be used only for small allocations that are not application driven. Preferabely the allocation sizes should be known at compile time. Careful when allocating memory for COM and other objects: The main object should not be allocated with the _nofail variants. Subsequent small allocations in the same object could use the _nofail version.
heap_alloc_zero was renamed to heap_zalloc to shorten it. Especially heap_alloc_zero_nofail would be too long. Same for heap_realloc_zero. The rename shouldn't be too painful: On a quick grep I found just a handfull of heap_realloc_zero and about 470 heap_alloc_zero. I'm also expecting that a good part of those heap_alloc_zero would be changed anyway to the _nofail variant or to heap_calloc.
Also adding "z" in front of the alloc function name is common: The Linux kernel uses kzalloc and $ man -k alloc_zero alloc_zero: nothing appropriate. $ man -k zalloc CRYPTO_secure_zalloc (3ssl) - secure heap storage CRYPTO_zalloc (3ssl) - Memory allocation functions OPENSSL_secure_zalloc (3ssl) - secure heap storage OPENSSL_zalloc (3ssl) - Memory allocation functions
heap_realloc variants follow the sane POSIX / C realloc behavior: - Passing size 0 will free the old memory. - Passing NULL as memory pointer will allocate new memory.
heap_free() returns void like free(): - No existing use of the heap_free() return value. - There are a few "return HeapFree()" statements but more than half of those are just in heap_free() wrappers. - The rest of "return HeapFree()" are in "destroy object" style functions, mostly in gdi32. Most are internal helpers but I didn't bother tracing them if the return value is needed or not.
Added heap_calloc() that matches the calloc behavior and always zeroes the memory. Thus there is no heap_zcalloc(). heap_calloc() (differently named) is already used in the D3D related dlls. I could also immediately make use of it in vbscript. I'm not sure if a heap_calloc_nofail would make sense. Can be added later if there is a real need for it.
Now let the bikeshedding begin!
include/Makefile.in | 1 + include/wine/heap.h | 127 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 include/wine/heap.h
diff --git a/include/Makefile.in b/include/Makefile.in index 430c4733af..d107cf7919 100644 --- a/include/Makefile.in +++ b/include/Makefile.in @@ -678,6 +678,7 @@ HEADER_SRCS = \ windowsx.h \ wine/debug.h \ wine/exception.h \ + wine/heap.h \ wine/library.h \ wine/unicode.h \ winerror.h \ diff --git a/include/wine/heap.h b/include/wine/heap.h new file mode 100644 index 0000000000..ec10db67be --- /dev/null +++ b/include/wine/heap.h @@ -0,0 +1,127 @@ +/* + * Wine heap memory allocation wrappers + * + * Copyright 2006 Jacek Caban for CodeWeavers + * Copyright 2013,2018 Michael Stefaniuc + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#ifndef __WINE_WINE_HEAP_H +#define __WINE_WINE_HEAP_H + +#include <windef.h> + +#ifdef __cplusplus +extern "C" { +#endif + +/* HeapAlloc wrappers + + * "zalloc" variants + * The returned memory is initialized with zero. + + * "nofail" variants + * The returned pointer is never NULL and an exception is raised in out of memory conditions. + * Use only for small allocations that are not application driven. + * Preferabely the allocation size should be known at compile time. + */ +static inline void * __WINE_ALLOC_SIZE(1) heap_alloc(SIZE_T len) +{ + return HeapAlloc(GetProcessHeap(), 0, len); +} + +static inline void * __WINE_ALLOC_SIZE(1) heap_alloc_nofail(SIZE_T len) +{ + return HeapAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, len); +} + +static inline void * __WINE_ALLOC_SIZE(1) heap_zalloc(SIZE_T len) +{ + return HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len); +} + +static inline void * __WINE_ALLOC_SIZE(1) heap_zalloc_nofail(SIZE_T len) +{ + return HeapAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS | HEAP_ZERO_MEMORY, len); +} + +/* HeapReAlloc wrappers with POSIX / C realloc() behavior: + * Passing size 0 will free the old memory. + * Passing NULL as memory pointer will allocate new memory. + + * "zalloc" variants + * The returned memory is initialized with zero. + + * "nofail" variants + * The returned pointer is NULL only when the size == 0 and the memory is freed, + * Raises an exception in out of memory conditions. + * Use only for small allocations that are not application driven. + * Preferabely the allocation size should be known at compile time. + */ +static inline void * __WINE_ALLOC_SIZE(2) _heap_realloc(void *mem, SIZE_T len, DWORD flags) +{ + if (len) { + if (mem) + return HeapReAlloc(GetProcessHeap(), flags, mem, len); + else + return HeapAlloc(GetProcessHeap(), flags, len); + } else { + HeapFree(GetProcessHeap(), 0, mem); + return NULL; + } +} + +static inline void * __WINE_ALLOC_SIZE(2) heap_realloc(void *mem, SIZE_T len) +{ + return _heap_realloc(mem, len, 0); +} + +static inline void * __WINE_ALLOC_SIZE(2) heap_realloc_nofail(void *mem, SIZE_T len) +{ + return _heap_realloc(mem, len, HEAP_GENERATE_EXCEPTIONS); +} + +static inline void * __WINE_ALLOC_SIZE(2) heap_zrealloc(void *mem, SIZE_T len) +{ + return _heap_realloc(mem, len, HEAP_ZERO_MEMORY); +} + +static inline void * __WINE_ALLOC_SIZE(2) heap_zrealloc_nofail(void *mem, SIZE_T len) +{ + return _heap_realloc(mem, len, HEAP_GENERATE_EXCEPTIONS | HEAP_ZERO_MEMORY); +} + +/* heap_calloc + * calloc() like wrapper that zeroes the returned memory. + */ +static inline void * __WINE_ALLOC_SIZE(2) heap_calloc(SIZE_T count, SIZE_T size) +{ + if (count > ~(SIZE_T)0 / size) + return NULL; + return HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, count * size); +} + + +static inline void heap_free(void *mem) +{ + HeapFree(GetProcessHeap(), 0, mem); +} + +#ifdef __cplusplus +} +#endif + +#endif /* __WINE_WINE_HEAP_H */
Hi Michael,
On 01/21/2018 10:05 PM, Michael Stefaniuc wrote:
Signed-off-by: Michael Stefaniuc mstefani@winehq.org
I know that Alexandre and Jacek wanted to work on the HeapAlloc wrappers. But I don't remember if I had promised (or not) at the last WineConf to dig up my old patch and polish it up and submit it to serve as a discussion start point. Anyway, here it is with some explanations.
Thanks for looking at this. This looks mostly good for me. I have a few suggestions to consider.
The _nofail variants should not "fail" memory allocations and will raise an exception on out of memory conditions. heap_alloc_nofail thus will never return NULL. heap_realloc_nofail can still return NULL but *only* when the alloc size is 0 and thus functions as heap_free. Checking the returned pointer for NULL is not needed when the size != 0. And the "assign to temp pointer to prevent mem leak on failure" workaround is never needed.
I think that most realloc use cases will use fallible variant (since its usage usually means that the buffer may grow) and they still need temporary pointer. How about using something like: BOOL heap_realloc(void **mem, SIZE_T size) instead? You could leave passed pointer alone on failure and set it to new memory on success.
The _nofail variants should be used only for small allocations that are not application driven. Preferabely the allocation sizes should be known at compile time. Careful when allocating memory for COM and other objects: The main object should not be allocated with the _nofail variants. Subsequent small allocations in the same object could use the _nofail version.
How about having just heap_alloc as infallible and heap_calloc as fallible? If you need fallible heap_alloc, you may just pass extra 1 argument to heap_calloc. If you need infallible heap_calloc, then you know there won't be an overflow and may just multiply arguments.
Thanks, Jacek
Hello Jacek,
On 01/22/2018 04:05 PM, Jacek Caban wrote:
On 01/21/2018 10:05 PM, Michael Stefaniuc wrote:
Signed-off-by: Michael Stefaniuc mstefani@winehq.org
I know that Alexandre and Jacek wanted to work on the HeapAlloc wrappers. But I don't remember if I had promised (or not) at the last WineConf to dig up my old patch and polish it up and submit it to serve as a discussion start point. Anyway, here it is with some explanations.
Thanks for looking at this. This looks mostly good for me. I have a few suggestions to consider.
thanks for reviewing the patch.
Before I reply to your suggestions here are two considerations that were at the base of my proposal:
Principle of the least surprise ------------------------------- - Developers are already used to the current wrappers. - Linux developers are used to the malloc, realloc, calloc functions while Windows developers are used to HeapAlloc(). The function names will confer an expectation of what the wrappers do. If possible we should fulfill that expectation. Ease the transition ------------------- - There should be no flag date for the move to the new wrappers. - Big churn patches should be avoided. - The full transition will take years, thus there will be new and old wrappers side by side. Those should have the same signature and the behavior should not differ wildly. - As I'll probably have to do a big part of this work you don't want me to run away screaming. And if I do, the code should be still in an acceptable consistent state.
The _nofail variants should not "fail" memory allocations and will raise an exception on out of memory conditions. heap_alloc_nofail thus will never return NULL. heap_realloc_nofail can still return NULL but *only* when the alloc size is 0 and thus functions as heap_free. Checking the returned pointer for NULL is not needed when the size != 0. And the "assign to temp pointer to prevent mem leak on failure" workaround is never needed.
I think that most realloc use cases will use fallible variant (since its usage usually means that the buffer may grow) and they still need temporary pointer. How about using something like: BOOL heap_realloc(void **mem, SIZE_T size) instead? You could leave passed pointer alone on failure and set it to new memory on success.
Alexandre's requirement was a terse "sane behavior like realloc".
I see your proposal has some merit. But why stop there? We could just free the old pointer on allocation failure while keeping the existing function signature. I didn't check but I would assume that is the common case when the temp pointer trick is used. That can be than simplified to just without leaking any memory: if (!(ptr = heap_realloc(ptr, size)) return E_OUTOFMEMORY;
We can have another wrapper heap_reallocex() that implements your suggestion for the cases when a different cleanup is needed. Of course with a better name for the wrapper.
Anyway, both change variants are possible from a churn point of view: wine$ git grep heap_realloc | wc -l 196
The _nofail variants should be used only for small allocations that are not application driven. Preferabely the allocation sizes should be known at compile time. Careful when allocating memory for COM and other objects: The main object should not be allocated with the _nofail variants. Subsequent small allocations in the same object could use the _nofail version.
How about having just heap_alloc as infallible and heap_calloc as fallible? If you need fallible heap_alloc, you may just pass extra 1 argument to heap_calloc. If you need infallible heap_calloc, then you know there won't be an overflow and may just multiply arguments.
You want to change
if (!(obj = heap_alloc(sizeof(*obj))) return E_OUTOFMEMORY;
to
if (!(obj = heap_calloc(sizeof(*obj), 1)) return E_OUTOFMEMORY;
?
Ugh, NACK. Quite a surprising change. Requires a flag date or big churn patches. The default heap_alloc() should keep its current behavior. That allows for a proper code review and transition to the infallible alloc behavior.
If somebody else has a better name for the "nofail" variants I'm all ears. The only other name I could came up that matched the intent was heap_alloc_I_reviewed_the_code_and_know_what_I_am_doing(); but that's even longer ;)
bye michael
On 01/22/2018 08:23 PM, Michael Stefaniuc wrote:
On 01/22/2018 04:05 PM, Jacek Caban wrote:
On 01/21/2018 10:05 PM, Michael Stefaniuc wrote:
Signed-off-by: Michael Stefaniuc mstefani@winehq.org
I know that Alexandre and Jacek wanted to work on the HeapAlloc wrappers. But I don't remember if I had promised (or not) at the last WineConf to dig up my old patch and polish it up and submit it to serve as a discussion start point. Anyway, here it is with some explanations.
Thanks for looking at this. This looks mostly good for me. I have a few suggestions to consider.
thanks for reviewing the patch.
Before I reply to your suggestions here are two considerations that were at the base of my proposal:
Principle of the least surprise
- Developers are already used to the current wrappers.
- Linux developers are used to the malloc, realloc, calloc functions
while Windows developers are used to HeapAlloc(). The function names will confer an expectation of what the wrappers do. If possible we should fulfill that expectation. Ease the transition
- There should be no flag date for the move to the new wrappers.
- Big churn patches should be avoided.
- The full transition will take years, thus there will be new and old
wrappers side by side. Those should have the same signature and the behavior should not differ wildly.
- As I'll probably have to do a big part of this work you don't want me
to run away screaming. And if I do, the code should be still in an acceptable consistent state.
The _nofail variants should not "fail" memory allocations and will raise an exception on out of memory conditions. heap_alloc_nofail thus will never return NULL. heap_realloc_nofail can still return NULL but *only* when the alloc size is 0 and thus functions as heap_free. Checking the returned pointer for NULL is not needed when the size != 0. And the "assign to temp pointer to prevent mem leak on failure" workaround is never needed.
I think that most realloc use cases will use fallible variant (since its usage usually means that the buffer may grow) and they still need temporary pointer. How about using something like: BOOL heap_realloc(void **mem, SIZE_T size) instead? You could leave passed pointer alone on failure and set it to new memory on success.
Alexandre's requirement was a terse "sane behavior like realloc".
I see your proposal has some merit. But why stop there? We could just free the old pointer on allocation failure while keeping the existing function signature. I didn't check but I would assume that is the common case when the temp
Actually I do have a coccinelle script that I used to gather some heap_realloc data. The script found:
realloc calls: heap_realloc: 122 HeapReAlloc: 401 heap_realloc_zero: 12 d3dcompiler_realloc: 8 msi_realloc_zero: 4 msi_realloc: 20
realloc leaks: 279 realloc temp variable: 270 return only: 15 free old mem: 8
The "leaks" are ptr = heap_realloc(ptr, size)
The "temp variable" is code like temp = heap_realloc(ptr, size)
Of those calls using a temp variable the "return only" just do temp = heap_realloc(ptr, size) if (!temp) return ...;
The "free old mem" ones free the old memory as their first action as in temp = heap_realloc(ptr, size) if (!temp) { heap_free(ptr); ... }
So for more than half of the HeapReAlloc / heap_realloc calls an "automatic free the old mem on failure" approach would work.
That is just a lower boundary as I didn't check all the permutations on how to check for a failed heap_realloc. E.g. my script doesn't checks in the "return only" or "free old mem" cases for code like if (!(temp = heap_realloc(ptr, size))
bye michael
pointer trick is used. That can be than simplified to just without leaking any memory: if (!(ptr = heap_realloc(ptr, size)) return E_OUTOFMEMORY;
We can have another wrapper heap_reallocex() that implements your suggestion for the cases when a different cleanup is needed. Of course with a better name for the wrapper.
Anyway, both change variants are possible from a churn point of view: wine$ git grep heap_realloc | wc -l 196
The _nofail variants should be used only for small allocations that are not application driven. Preferabely the allocation sizes should be known at compile time. Careful when allocating memory for COM and other objects: The main object should not be allocated with the _nofail variants. Subsequent small allocations in the same object could use the _nofail version.
How about having just heap_alloc as infallible and heap_calloc as fallible? If you need fallible heap_alloc, you may just pass extra 1 argument to heap_calloc. If you need infallible heap_calloc, then you know there won't be an overflow and may just multiply arguments.
You want to change
if (!(obj = heap_alloc(sizeof(*obj))) return E_OUTOFMEMORY;
to
if (!(obj = heap_calloc(sizeof(*obj), 1)) return E_OUTOFMEMORY;
?
Ugh, NACK. Quite a surprising change. Requires a flag date or big churn patches. The default heap_alloc() should keep its current behavior. That allows for a proper code review and transition to the infallible alloc behavior.
If somebody else has a better name for the "nofail" variants I'm all ears. The only other name I could came up that matched the intent was heap_alloc_I_reviewed_the_code_and_know_what_I_am_doing(); but that's even longer ;)
bye michael
Hello Michael,
On 01/22/2018 08:23 PM, Michael Stefaniuc wrote:
Hello Jacek,
On 01/22/2018 04:05 PM, Jacek Caban wrote:
On 01/21/2018 10:05 PM, Michael Stefaniuc wrote:
Signed-off-by: Michael Stefaniuc mstefani@winehq.org
I know that Alexandre and Jacek wanted to work on the HeapAlloc wrappers. But I don't remember if I had promised (or not) at the last WineConf to dig up my old patch and polish it up and submit it to serve as a discussion start point. Anyway, here it is with some explanations.
Thanks for looking at this. This looks mostly good for me. I have a few suggestions to consider.
thanks for reviewing the patch.
Before I reply to your suggestions here are two considerations that were at the base of my proposal:
Principle of the least surprise
- Developers are already used to the current wrappers.
- Linux developers are used to the malloc, realloc, calloc functions
while Windows developers are used to HeapAlloc(). The function names will confer an expectation of what the wrappers do. If possible we should fulfill that expectation. Ease the transition
- There should be no flag date for the move to the new wrappers.
- Big churn patches should be avoided.
- The full transition will take years, thus there will be new and old
wrappers side by side. Those should have the same signature and the behavior should not differ wildly.
- As I'll probably have to do a big part of this work you don't want me
to run away screaming. And if I do, the code should be still in an acceptable consistent state.
The _nofail variants should not "fail" memory allocations and will raise an exception on out of memory conditions. heap_alloc_nofail thus will never return NULL. heap_realloc_nofail can still return NULL but *only* when the alloc size is 0 and thus functions as heap_free. Checking the returned pointer for NULL is not needed when the size != 0. And the "assign to temp pointer to prevent mem leak on failure" workaround is never needed.
I think that most realloc use cases will use fallible variant (since its usage usually means that the buffer may grow) and they still need temporary pointer. How about using something like: BOOL heap_realloc(void **mem, SIZE_T size) instead? You could leave passed pointer alone on failure and set it to new memory on success.
Alexandre's requirement was a terse "sane behavior like realloc".
I see your proposal has some merit. But why stop there? We could just free the old pointer on allocation failure while keeping the existing function signature. I didn't check but I would assume that is the common case when the temp pointer trick is used. That can be than simplified to just without leaking any memory: if (!(ptr = heap_realloc(ptr, size)) return E_OUTOFMEMORY;
We can have another wrapper heap_reallocex() that implements your suggestion for the cases when a different cleanup is needed. Of course with a better name for the wrapper.
Anyway, both change variants are possible from a churn point of view: wine$ git grep heap_realloc | wc -l 196
The code in your example is indeed nicer. However, I have mixed feelings about it mostly because it would look like a familiar realloc function, but have a very signifiant difference. This may be misleading, I wouldn't expect such function to free memory. It's not a strong opinion through.
The _nofail variants should be used only for small allocations that are not application driven. Preferabely the allocation sizes should be known at compile time. Careful when allocating memory for COM and other objects: The main object should not be allocated with the _nofail variants. Subsequent small allocations in the same object could use the _nofail version.
How about having just heap_alloc as infallible and heap_calloc as fallible? If you need fallible heap_alloc, you may just pass extra 1 argument to heap_calloc. If you need infallible heap_calloc, then you know there won't be an overflow and may just multiply arguments.
You want to change
if (!(obj = heap_alloc(sizeof(*obj))) return E_OUTOFMEMORY;
to
if (!(obj = heap_calloc(sizeof(*obj), 1)) return E_OUTOFMEMORY;
?
Ugh, NACK. Quite a surprising change. Requires a flag date or big churn patches. The default heap_alloc() should keep its current behavior. That allows for a proper code review and transition to the infallible alloc behavior.
Sure, fine with me.
If somebody else has a better name for the "nofail" variants I'm all ears. The only other name I could came up that matched the intent was heap_alloc_I_reviewed_the_code_and_know_what_I_am_doing(); but that's even longer ;)
heap_alloc_infallible_makes_allocation_trivial_so_lets_give_developer_time_to_think_about_it_while_typing() ? ;)
Thanks, Jacek
FWIW it generally sounds good to me. I like Jacek's idea for heap_realloc(), at least on paper.
But since you wanted bikeshedding...
2018-01-21 22:05 GMT+01:00 Michael Stefaniuc mstefani@winehq.org:
diff --git a/include/wine/heap.h b/include/wine/heap.h new file mode 100644 index 0000000000..ec10db67be --- /dev/null +++ b/include/wine/heap.h @@ -0,0 +1,127 @@ +/*
- Wine heap memory allocation wrappers
- Copyright 2006 Jacek Caban for CodeWeavers
- Copyright 2013,2018 Michael Stefaniuc
Missing space after comma.
- "nofail" variants
The returned pointer is never NULL and an exception is raised in out of memory conditions.
Use only for small allocations that are not application driven.
Preferabely the allocation size should be known at compile time.
"Preferably".
/me runs
-
Jacek Caban -
Matteo Bruni -
Michael Stefaniuc