Re: [v5 PATCH 1/3] dwrite: Protect cached fontface list when accessed from multiple threads
On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)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 | | | | | Release | | | | Decrement | | free(cached) | | | | factory_unlock | | | | | | use after free (cached) | +----------------+------------------------+-------------------------+
On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov <theli.ua(a)gmail.com> wrote:
On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)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 | | | | | 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...
On 17.08.2017 07:11, Anton Romanov wrote:
On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov <theli.ua(a)gmail.com> wrote:
On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)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?
On Thu, Aug 17, 2017 at 12:40 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
On 17.08.2017 07:11, Anton Romanov wrote:
On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov <theli.ua(a)gmail.com> wrote:
On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)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?
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.
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
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 <theli.ua(a)gmail.com> wrote:
On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)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?
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.
On Thu, Aug 17, 2017 at 11:14 AM, Nikolay Sivov <bunglehead(a)gmail.com> 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 <theli.ua(a)gmail.com> wrote:
On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)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(-)
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.
On Thu, Aug 17, 2017 at 9:35 PM, Anton Romanov <theli.ua(a)gmail.com> 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.
participants (3)
-
Anton Romanov -
Nikolay Sivov -
Sebastian Lackner