[PATCH 1/3] msvcrt: Lock the heap in _callnewh

[Martin Storsjö]

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