[PATCH] Potential reference count races

[Max TenEyck Woodbury]

On 10/28/2012 12:06 PM, Nikolay Sivov wrote:
> On 10/28/2012 17:44, Max TenEyck Woodbury wrote:
>> On 10/28/2012 02:40 AM, Nikolay Sivov wrote:
>>> On 10/28/2012 04:59, max at mtew.isa-geek.net wrote:
>>>> From: Max TenEyck Woodbury <max at mtew.isa-geek.net>
>>>>
>>>> I have been looking at the Microsoft COM and related documentations
>>>> and noticed that they emphatically recommend using the Interlocked...
>>>> functions when manipulating reference counts.  I managed to set up a
>>>> search that showed where many of the reference count updates occur and
>>>> was somewhat surprised at how often this advice was not followed.
>>> 
>>> It doesn't mean it always has to be followed.
>> 
>> True in a limited sense, but there is a good reason behind their
>> recommendation.  Unless there is a good reason not to do so, this
>> particular piece of advice should be followed.
> 
> COM objects in wine follow this recommendation in general, even object
> itself is not thread safe.
> This doesn't mean however that you need this every time you have some
> kind of refcount of any sort.

It may or may not be necessary every time, but it should be demonstrated
that it is not necessary rather than assumed that it is not.  This is a
'race condition' after all, and they are known to be rare and difficult
to isolate.  I think it is good practice to assume there could be a race
problem rather than otherwise.
>>
>>>> While I have not converted every reference count update to use the
>>>> Interlocked... functions, this set of patches fixes a fair number of
>>>> them.
>>>>
>>>> These are not associated with any particular bug report; they are
>>>> simply a general precaution against operations that are known to be
>>>> associated with race conditions.
>>> 
>>> This precaution doesn't work in general. It's not enough to atomically
>>> update refcount to make an implementation thread safe. Also not
>>> everything is supposed to be thread safe in a first place.
>>>
>> First, explain what does not have to be thread safe.
> 
> Anything really, COM objects in particular if you were talking about them.
>
I think you are talking about the apartment model here, which forces
thread serialization.  Despite that, the Microsoft documentation still
recommends interlocked operations on reference counts...

>> I believe that application may try to use multiple threads anywhere,
>> so everything that can be made thread safe, should be.

> No.

What do you mean 'No'.  That is an opinion,  If you disagree, please
explain why.

>> Using interlocks on the reference count updates is a necessary step 
>> for thread safety.  You are correct that it is not sufficient, but
>> it is necessary.

> Again, it depends.

Yes, it might depend on many factors, most of which will make it safe
to not interlock, but it is a lot of work checking that all those
factors necessary to not use the interlock are in fact in place.
Further, if enough of those factors get changed, you have a problem.
This is what make code fragile.  Fragile can be avoided with steps
like this.
>>
>>> Changes like this:
>>>
>>>> -        for (i=0;i<howmuch;i++)
>>>> +        for (i=0;i<howmuch;++i)
>>>>            TRACE("notify at %d to %p\n",
>>>>                notify[i].dwOffset,notify[i].hEventNotify);
>>> 
>>> are not helpful at all.
>>> 
>> The post increment and decrement operation are specified as saving
>> the original value for use in the evaluation of the expression they
>> are part of and modifying the underlying stored value.  In expressions
>> like this, that saved value is then discarded.  The optimization phase
>> of the compilation usually removes both the save and discard operations.
> 
> Sure, but I don't think it's enough to justify such changes all over the
> place, in existing code.
> 
I agree that it is not enough to justify a separate set of patches, but
as part of another set of changes, I think it is justified.  After all,
how else are examples of bad code going to be removed.