Fix use-after-free bug in registry_watcher

[Roy Arad (Royar13)]

Fix the following scenario:

1. (Thread A) registry_watcher_t::callback() starts (before reaching TryAddRef), m_refCount = 1.
2. (Thread B) registry_watcher_state object is deleted (Release() method).
3. (Thread B) m_refCount is destroyed.
4. (Thread A) TryAddRef() is reached and accesses destroyed member m_refCount -> Use-after-free, UNDEFINED BEHAVIOR, can corrupt other members (which happened in production code using WIL).

[David Machaj (dmachaj)]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Chris Guzak (ChrisGuzak)]

looks good. do you have a synthetic test that shows the problem and validates the fix?

[Duncan Horn (dunhor)]

This suggests you are guarding against negative values. If you've seen this actually happen, we'd be very interested to learn more since that would suggest there's a mis-match of AddRef and Release.

[Duncan Horn (dunhor)]

Something seems fishy here. unique_threadpool_wait calls WaitForThreadpoolWaitCallbacks in its destructor, so scenario doesn't seem possible, assuming everything is working as designed. In particular, registry_watcher_state cannot be fully deleted during an invocation of the callback.

You do mention:

TryAddRef() is reached and accesses destroyed member m_refCount

which may technically have some merit to it since m_refCount is declared afterm_threadPoolWait in the definition of registry_watcher_state. That said, longs are trivially destructible, so this is only a "technically wrong" thing and unlikely to be observable in practice, outside of something like a Debug build or ASan/UBSan etc.

I'll second Chris' question - I'd like to know more about the scenario where you've seen issues.