[PATCH 1/3] msvcrt: Lock the heap in _callnewh
martin at martin.st
Fri Nov 6 06:43:59 CST 2015
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;
>>>> - (*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.
More information about the wine-devel