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

[Anton Romanov]

On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov  wrote:
> Signed-off-by: Nikolay Sivov
>
> 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              |                         |
|                |                        | Release                 |
|                |                        | Decrement               |
| free(cached)   |                        |                         |
| factory_unlock |                        |                         |
|                |                        | use after free (cached) |
+----------------+------------------------+-------------------------+

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

[Anton Romanov]

On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov  wrote:
> On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov  wrote:
> > Signed-off-by: Nikolay Sivov
> >
> > 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              |                         |
> |                |                        | 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)
004c:trace:dwrite:dwritefontface_ReleaseFontTable (0x97936c8)->((nil))
004c:trace:dwrite:dwritefontface_TryGetFontTable (0x97936c8)->("glyf"
0x339e00 0x339e08 0x339e04 0x339dfc)
004c:trace:dwrite:dwritefontface_ReleaseFontTable (0x97936c8)->((nil))
004c:trace:dwrite:dwritefontface_TryGetFontTable (0x97936c8)->("CFF "
0x339e00 0x339e08 0x339e04 0x339dfc)
004c:trace:dwrite:dwritefontface_TryGetFontTable (0x97936c8)->("COLR"
0x339e00 0x339e08 0x339e04 0x339dfc)
004c:trace:dwrite:dwritefontface_TryGetFontTable (0x97936c8)->("SVG "
0x339e00 0x339e08 0x339e04 0x339dfc)
004c:trace:dwrite:dwritefontface_TryGetFontTable (0x97936c8)->("sbix"
0x339e00 0x339e08 0x339e04 0x339dfc)
004c:trace:dwrite:dwritetextanalyzer_GetGlyphPlacements (L"1" 0x33b264
0x33b444 1 0x9636198 0x9635dc0 1 0x97936c8 16.00 0 0 "Zyyy" L"en-US"
0x95f5760 0x70456084 1 0x96ef808 0xd231630)
004c:trace:dwrite:dwritefontface_QueryInterface
(0x97936c8)->({a71efdb4-9fdb-4838-ad90-cfc3be8c3daf} 0x339fd0)
004c:trace:dwrite:dwritefontface_GetMetrics (0x97936c8)->(0x339fd8)
004c:trace:dwrite:dwritefontface1_GetDesignGlyphAdvances
(0x97936c8)->(1 0x9636198 0x339fd4 0)
004c:trace:dwrite:dwritefontface_Release (0x97936c8)->(1)
004c:trace:dwrite:dwritefontface_GetDesignGlyphMetrics
(0x97936c8)->(0x7045637c 1 0x7045638c 0)
004c:trace:dwrite:dwritefontface_GetSimulations (0x97936c8)
004c:trace:dwrite:dwritefontface_Release (0x97936c8)->(0)
wine: Unhandled page fault on read access to 0xffffffff at address
0x37719c8 (thread 004c), starting debugger...

[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  wrote:
> > On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov  wrote:
> > > Signed-off-by: Nikolay Sivov
> > >
> > > 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?

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

[Anton Romanov]

On Thu, Aug 17, 2017 at 12:40 AM, Sebastian Lackner
 wrote:
> On 17.08.2017 07:11, Anton Romanov wrote:
> > On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov  wrote:
> > > On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov  wrote:
> > > > Signed-off-by: Nikolay Sivov
> > > >
> > > > 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?

Sorry, I did misread the patch a bit. I'll take a closer look after I
have some sleep.
Here is the full lifetime of that fontface grepped from the log:
https://paste.ee/p/3No74  (  egrep "(0x97936c8|warn)"
~/Desktop/Magic_Crash.log )
And at the very list I cannot reproduce this crash on either windows
or wine with v1 of this patch.

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

[Sebastian Lackner]

On 17.08.2017 10:11, Anton Romanov wrote:
> > 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?
>
> Sorry, I did misread the patch a bit. I'll take a closer look after I
> have some sleep.
> Here is the full lifetime of that fontface grepped from the log:
> https://paste.ee/p/3No74  (  egrep "(0x97936c8|warn)"
> ~/Desktop/Magic_Crash.log )
> And at the very list I cannot reproduce this crash on either windows
> or wine with v1 of this patch.

The log contains the following line:

004c:warn:dwrite:factory_get_cached_fontface Failed to get {27f2a904-4eb8-441d-9678-0563f53e3e2f} from fontface, hr 0x80004005.

This means the cached interface was not used, and it is unclear where the pointer
in the following line comes from. Maybe I'm missing something, but for me it still
looks like a use-after-free. It does not really mean much that v1 "fixed" it,
sometimes a small timing difference is sufficient to hide the bug. ;)

Best regards,
Sebastian

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

[Nikolay Sivov]

On 17.08.2017 10:40, Sebastian Lackner wrote:
> On 17.08.2017 07:11, Anton Romanov wrote:
> > On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov  wrote:
> > > On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov  wrote:
> > > > Signed-off-by: Nikolay Sivov
> > > >
> > > > 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?

Yes, it's possible there's some MRU list as well. I'm almost sure
factory does not keep a reference, at least not in a way IUnknown
exposes it, I'll need to check if we have tests for that, and maybe add
some.
>

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

[Anton Romanov]

On Thu, Aug 17, 2017 at 11:14 AM, Nikolay Sivov  wrote:
> On 17.08.2017 10:40, Sebastian Lackner wrote:
> > On 17.08.2017 07:11, Anton Romanov wrote:
> > > On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov  wrote:
> > > > On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov  wrote:
> > > > > Signed-off-by: Nikolay Sivov
> > > > >
> > > > > dlls/dwrite/dwrite_private.h | 15 ++++++++---
> > > > >  dlls/dwrite/font.c           | 30 +++++++++++++--------
> > > > >  dlls/dwrite/main.c           | 63 +++++++++++++++++++++++++++++++-------------
> > > > >  3 files changed, 74 insertions(+), 34 deletions(-)

So I dived a bit deeper into that and it looks like there is now a
race for fontfile:

004c:trace:dwrite:dwritefontface_Release (0x189c09a0)->(0)
005c:trace:dwrite:dwritefontface_GetSimulations (0x189c09a0)
005c:trace:dwrite:dwritefontface_GetFiles (0x189c09a0)->(0xa5fe400 0xa5fe408)
005c:trace:dwrite:dwritefontfile_AddRef (0x15cf7f00)->(3)
005c:trace:dwrite:dwritefontfile_GetReferenceKey
(0x15cf7f00)->(0xa5fe404, 0xa5fe3fc)
005c:trace:dwrite:dwritefontfile_Release (0x15cf7f00)->(2)
005c:trace:dwrite:dwritefontface_QueryInterface
(0x189c09a0)->({27f2a904-4eb8-441d-9678-0563f53e3e2f} 0xa5fe57c)
005c:warn:dwrite:factory_get_cached_fontface Failed to get
{27f2a904-4eb8-441d-9678-0563f53e3e2f} from fontface, hr 0x80004005.
005c:trace:dwrite:factory_get_cached_fontface returning cached
fontface 0x189c09a0
005c:trace:dwrite:shareddwritefactory_AddRef (0x3a0cf18)
004c:trace:dwrite_file:localfontfilestream_Release (0x1633b128)->(0)
005c:trace:dwrite:dwritefontfile_GetLoader (0x15cf7f00)->(0xa5fe408)
005c:trace:dwrite:localfontfileloader_AddRef (0x15cf5108)->(225)
005c:trace:dwrite:dwritefontfile_GetReferenceKey
(0x15cf7f00)->(0xa5fe410, 0xa5fe40c)
005c:trace:dwrite:localfontfileloader_CreateStreamFromKey
(0x15cf5108)->(0x15cf8fc0, 64, 0x1633aff8)
005c:trace:dwrite:localfontfileloader_CreateStreamFromKey name:
L"C:\windows\fonts\Ariali.TTF"
005c:trace:dwrite_file:localfontfilestream_AddRef (0x1633b128)->(1)
005c:trace:dwrite:localfontfileloader_Release (0x15cf5108)->(224)
005c:trace:dwrite:get_stream_from_file 0x15cf7f00 -> 0x1633b128
005c:trace:dwrite:dwritefontfile_AddRef (0x15cf7f00)->(3)
005c:trace:dwrite_file:localfontfilestream_ReadFileFragment
(0x1633b128)->(0xa5fe318, 0x0, 0xc, 0xa5fe31c)

The crash is in thread 5c with backtrace being:
=>0 0x7d442918 opentype_get_font_table+0xa8(stream_desc=0xa5fe410,
tag=0x322f534f, table_data=0xa5fe39c, table_context=0xa5fe38c,
table_size=0x0(nil), found=0x0(nil))
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../../../wine/dlls/dwrite/opentype.c:1075]
in dwrite (0x0a5fe338)
  1 0x7d442f0e opentype_get_font_metrics+0x5d(stream_desc=<is not
available>, metrics=<is not available>, caret=<is not available>)
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../../../wine/dlls/dwrite/opentype.c:1209]
in dwrite (0x0a5fe3b8)
  2 0x7d42a089 create_fontface+0x2c8(desc=0xa5fe470,
cached_list=0x3a0cf34, ret=0xa5fe57c)
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../../../wine/dlls/dwrite/font.c:4354]
in dwrite (0x0a5fe438)
  3 0x7d42a396 get_fontface_from_font.isra+0xb5() in dwrite (0x0a5fe4a8)
  4 0x7d42a420 dwritefont3_CreateFontFace+0x5f(iface=<couldn't compute
location>, fontface=<couldn't compute location>)
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../../../wine/dlls/dwrite/font.c:1674]
in dwrite (0x0a5fe4d8)
  5 0x7d41d42b dwritefont_CreateFontFace+0x5a(iface=<couldn't compute
location>, fontface=<couldn't compute location>)
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../include/dwrite_3.h:1208]
in dwrite (0x0a5fe518)

And the reason it didn't crash with v1 is that the whole chain of
releases (from fontface to factory, file, etc) was protected by lock.
While this version releases the lock after removing fontface from the
cache.

Also with this patch the line
TRACE("returning cached fontface %p\n", cached->fontface);
Should probably be under the else branch as it outputs this regardless
of whether it actually acquired a reference.

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

[Anton Romanov]

On Thu, Aug 17, 2017 at 9:35 PM, Anton Romanov  wrote:
> And the reason it didn't crash with v1 is that the whole chain of
> releases (from fontface to factory, file, etc) was protected by lock.
> While this version releases the lock after removing fontface from the
> cache.

Sorry, this is wrong.
It actually should've crashed even with v1 (and now that I think of it
I think I did get couple of crashes in this codepath even with v1).
So I guess with this patch for w/e reason timing have changed and that
different race became more likely.
And this is fine, but fontfile should be protected as well.