1. 18 May, 2023 3 commits
    • Bartosz Kosiorek's avatar
    • Jinoh Kang's avatar
      combase: Prevent use-after-free due to a race with proxy_manager_destroy. · e308b19a
      Jinoh Kang authored
      Today, find_proxy_manager() may return a proxy manager that is about to
      be freed.  This happens when the proxy manager's reference count reaches
      zero, but proxy_manager_destroy() has not removed the proxy manager from
      the apartment proxy list.
      
      Fix this by incrementing the reference count only if it is already
      nonzero.  If the reference count is zero, any reference to the proxy
      manager will become invalid after the current thread leaves the critical
      section (apt->cs).  This is because proxy_manager_destroy() will proceed
      to destroy the proxy manager after the apartment lock (apt->cs) is
      released.
      
      An alternative solution would be to prevent find_proxy_manager from
      observing the zero reference count in the first place.  Multiple
      approaches have been considered for implementing this solution, but were
      eventually dropped due to several disadvantages when applied to the
      current Wine codebase:
      
      1. Always acquire the apartment lock from the proxy manager's Release()
         method, and hold the lock until the proxy manager is completely
         removed from the list if the reference count drops to zero.
      
         This requires handling the special case when the proxy manager's
         parent apartment has been destroyed.  When an apartment is destroyed,
         it sets the `parent` field of each proxy manager that was previously
         owned by the apartment to NULL.  This means that each access to
         `This->parent->cs` has to be guarded by a NULL check for
         `This->parent`.
      
         Even if `parent` were never NULL, unconditionally acquiring a lock
         may unnecessarily block the subroutine and introduce contention.
      
      2. Opportunistically decrement the reference count without holding the
         lock, but only if the count is greater than 1.  This approach is
         still not free from the NULL parent apartment problem.
      
      3. Check for any concurrent reference count change from
         proxy_manager_destroy(), and bail out if such change has occurred.
         This makes it possible for the proxy manager's AddRef() method to
         return 1, which is unusual.
      e308b19a
    • Jinoh Kang's avatar
      combase: Compare AddRef() return value against 1 instead of 0 in find_proxy_manager. · f55f0b83
      Jinoh Kang authored
      Today, find_proxy_manager() tests if AddRef() returns 0 in an attempt to
      protect against a race condition bug.
      
      Note that AddRef does not return zero in normal circumstances, since
      AddRef returns the reference count after the increment, not before.
      
      Fix this by comparing the return value of AddRef() against 1, not 0.
      f55f0b83
  2. 17 May, 2023 11 commits
  3. 16 May, 2023 26 commits