[v5 PATCH 1/3] dwrite: Protect cached fontface list when accessed from multiple threads

[Sebastian Lackner]

On 17.08.2017 07:11, Anton Romanov wrote:
> On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov <theli.ua at gmail.com> wrote:
>> On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov at codeweavers.com> wrote:
>>> Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
>>> ---
>>>  dlls/dwrite/dwrite_private.h | 15 ++++++++---
>>>  dlls/dwrite/font.c           | 30 +++++++++++++--------
>>>  dlls/dwrite/main.c           | 63 +++++++++++++++++++++++++++++++-------------
>>>  3 files changed, 74 insertions(+), 34 deletions(-)
>>>
>> Since apps seem to freely use fontface interfaces from multiple
>> threads - Isn't the following race possible?
>>
>> +----------------+------------------------+-------------------------+
>> |       T1       |           T2           |           T3            |
>> +----------------+------------------------+-------------------------+
>> | Release        |                        |                         |
>> | Decrement      |                        |                         |
>> | == 0 -> True   |                        |                         |
>> |                | Query_Interface/AddRef |                         |
>> |                | Increment              |                         |

After the last Release call the app should not be calling any function on
the object directly.

The only valid code path to obtain a new reference is going through
factory_get_cached_fontface. The code in this function is written in such
a way that it only uses very specific safe methods. Most importantly,
destroyed font faces are skipped, so for properly written apps no race
conditions are possible.

>> |                |                        | Release                 |
>> |                |                        | Decrement               |
>> | free(cached)   |                        |                         |
>> | factory_unlock |                        |                         |
>> |                |                        | use after free (cached) |
>> +----------------+------------------------+-------------------------+
> Actually, with this version of the patch looks like this is exactly
> the crash I'm consistently getting now (was fine with v1):
> 
> 0093:trace:dwrite:dwritefontface_Release (0x97936c8)->(1)
> 0093:trace:dwrite:dwritefontface_Release (0x97936c8)->(0)
> 004c:trace:dwrite:dwritefontface_GetFiles (0x97936c8)->(0x339d58 0x339d50)
> 004c:trace:dwrite:dwritefontface_GetIndex (0x97936c8)
> 004c:trace:dwrite:dwritefontface_TryGetFontTable (0x97936c8)->("GSUB"
> 0x339e60 0x339e68 0x339e64 0x339e5c)

It is interesting that there is no "returning cached fontface" in the log
above. This suggests that your interpretation of the error is wrong, the
app basically operates on a destroyed object (or there is an error somewhere
else).

Does this really work reliable on Windows? If yes it means the current idea
of thread safety is not really sufficient. Maybe Windows preserves cache
entries for some specific time (or for the duration of the factory), even
after the last reference is gone? Or maybe the factory has a reference on
each font face?