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

[Piotr Caban]

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