Re: [PATCH 1/3] msvcrt: Lock the heap in _callnewh
On 11/06/15 11:28, Martin Storsjo wrote:
int CDECL _callnewh(MSVCRT_size_t size) { + int ret = 0; + LOCK_HEAP; if(MSVCRT_new_handler) - (*MSVCRT_new_handler)(size); - return 0; + ret = (*MSVCRT_new_handler)(size); + UNLOCK_HEAP; + return ret; } It doesn't make sense to lock heap cs in this case. Native is not doing it (at least in case of msvcp90.dll). The return value change looks almost correct - the function only returns 0 or 1.
Thanks, Piotr
On Fri, 6 Nov 2015, Piotr Caban wrote:
On 11/06/15 11:28, Martin Storsjo wrote:
int CDECL _callnewh(MSVCRT_size_t size) { + int ret = 0; + LOCK_HEAP; if(MSVCRT_new_handler) - (*MSVCRT_new_handler)(size); - return 0; + ret = (*MSVCRT_new_handler)(size); + UNLOCK_HEAP; + return ret; } It doesn't make sense to lock heap cs in this case. Native is not doing it (at least in case of msvcp90.dll).
Hmm, but wouldn't it risk a race with another thread doing set_new_handler then? OTOH I guess that's ok/expected? But there's a similar lock in MSVCRT_operator_new; should we do the same locking on the msvcp side instead then, or just skip it altogether?
The return value change looks almost correct - the function only returns 0 or 1.
Ok, so should I force the return value to 0 or 1 regardless of what the function returns? I.e. ret = (*MSVCRT_new_handler)(size) ? 1 : 0; (or !!), or should I just repost it without the locking? // Martin
On 11/06/15 11:53, Martin Storsjö wrote:
On Fri, 6 Nov 2015, Piotr Caban wrote:
On 11/06/15 11:28, Martin Storsjo wrote:
int CDECL _callnewh(MSVCRT_size_t size) { + int ret = 0; + LOCK_HEAP; if(MSVCRT_new_handler) - (*MSVCRT_new_handler)(size); - return 0; + ret = (*MSVCRT_new_handler)(size); + UNLOCK_HEAP; + return ret; } It doesn't make sense to lock heap cs in this case. Native is not doing it (at least in case of msvcp90.dll).
Hmm, but wouldn't it risk a race with another thread doing set_new_handler then? OTOH I guess that's ok/expected? Yes, there's a possibility of race in wine code. It can be easily avoided by storing new_handler in local variable in callnewh. Then it will be possible to call old handler while new handler was set. I think that it's better then possible crash if new_handler is set to NULL.
But there's a similar lock in MSVCRT_operator_new; should we do the same locking on the msvcp side instead then, or just skip it altogether? I guess that operator_new should use callnewh without locking the heap section. It can be easily tested by setting new_handler that takes lots of time to finish and trying to allocate big memory chunks in 2 threads. If second new_handler is called while first is still executed than there's no lock in native library (it's not a test that can go into wine because it depends on allocation failure).
The return value change looks almost correct - the function only returns 0 or 1.
Ok, so should I force the return value to 0 or 1 regardless of what the function returns? I.e. ret = (*MSVCRT_new_handler)(size) ? 1 : 0; Yes, this is what native does.
Thanks, Piotr
On Fri, 6 Nov 2015, Piotr Caban wrote:
On 11/06/15 11:53, Martin Storsjö wrote:
On Fri, 6 Nov 2015, Piotr Caban wrote:
On 11/06/15 11:28, Martin Storsjo wrote:
int CDECL _callnewh(MSVCRT_size_t size) { + int ret = 0; + LOCK_HEAP; if(MSVCRT_new_handler) - (*MSVCRT_new_handler)(size); - return 0; + ret = (*MSVCRT_new_handler)(size); + UNLOCK_HEAP; + return ret; } It doesn't make sense to lock heap cs in this case. Native is not doing it (at least in case of msvcp90.dll).
Hmm, but wouldn't it risk a race with another thread doing set_new_handler then? OTOH I guess that's ok/expected? Yes, there's a possibility of race in wine code. It can be easily avoided by storing new_handler in local variable in callnewh. Then it will be possible to call old handler while new handler was set. I think that it's better then possible crash if new_handler is set to NULL.
But there's a similar lock in MSVCRT_operator_new; should we do the same locking on the msvcp side instead then, or just skip it altogether? I guess that operator_new should use callnewh without locking the heap section. It can be easily tested by setting new_handler that takes lots of time to finish and trying to allocate big memory chunks in 2 threads. If second new_handler is called while first is still executed than there's no lock in native library (it's not a test that can go into wine because it depends on allocation failure).
Ok, I tested this, and indeed, two threads can simultaneously execute the new handler. So I'll send a patch for removing the lock from MSVCRT_operator_new then as well. // Martin
participants (2)
-
Martin Storsjö -
Piotr Caban