[PATCH v2 0/2] MR2302: gdiplus: Use FILE_SHARE_READ in GdipPrivateAddFontFile().
ZusiDisplay sometimes loads the same font file into two different PrivateFontCollections using two threads, so there is a race condition when the file is opened without the FILE_SHARE_READ sharing mode. The second call to GdipPrivateAddFontFile() might fail if the first one hasn't closed the file handle yet. -- v2: gdiplus: Use FILE_SHARE_READ in GdipPrivateAddFontFile(). gdiplus/tests: Test for GdipPrivateAddFontFile() sharing violation. https://gitlab.winehq.org/wine/wine/-/merge_requests/2302
From: Florian Will <florian.will(a)gmail.com> --- dlls/gdiplus/tests/font.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/dlls/gdiplus/tests/font.c b/dlls/gdiplus/tests/font.c index ccdb7a2ef96..7fbe619cd93 100644 --- a/dlls/gdiplus/tests/font.c +++ b/dlls/gdiplus/tests/font.c @@ -76,6 +76,7 @@ static void test_long_name(void) WCHAR path[MAX_PATH]; GpStatus stat; GpFontCollection *fonts; + HANDLE file; INT num_families; GpFontFamily *family, *cloned_family; WCHAR family_name[LF_FACESIZE]; @@ -86,8 +87,19 @@ static void test_long_name(void) create_testfontfile(L"wine_longname.ttf", 1, path); + file = CreateFileW(path, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); + ok(file != INVALID_HANDLE_VALUE, "CreateFileW failed: %ld\n", GetLastError()); + stat = GdipPrivateAddFontFile(fonts, path); - ok(stat == Ok, "GdipPrivateAddFontFile failed: %d\n", stat); + todo_wine ok(stat == Ok, "GdipPrivateAddFontFile failed with open file handle: %d\n", stat); + + CloseHandle(file); + + if (stat != Ok) { + /* try again without opened file handle */ + stat = GdipPrivateAddFontFile(fonts, path); + ok(stat == Ok, "GdipPrivateAddFontFile failed: %d\n", stat); + } stat = GdipGetFontCollectionFamilyCount(fonts, &num_families); ok(stat == Ok, "GdipGetFontCollectionFamilyCount failed: %d\n", stat); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2302
From: Florian Will <florian.will(a)gmail.com> ZusiDisplay sometimes loads the same font file into two different PrivateFontCollections using two threads, so there is a race condition when the file is opened without the FILE_SHARE_READ sharing mode. The second call to GdipPrivateAddFontFile() might fail if the first one hasn't closed the file handle yet. --- dlls/gdiplus/font.c | 2 +- dlls/gdiplus/tests/font.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/gdiplus/font.c b/dlls/gdiplus/font.c index 96d3c5630bc..bcf36a60090 100644 --- a/dlls/gdiplus/font.c +++ b/dlls/gdiplus/font.c @@ -1110,7 +1110,7 @@ GpStatus WINGDIPAPI GdipPrivateAddFontFile(GpFontCollection *collection, GDIPCON if (!collection || !name) return InvalidParameter; - file = CreateFileW(name, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL); + file = CreateFileW(name, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); if (file == INVALID_HANDLE_VALUE) return InvalidParameter; if (!GetFileSizeEx(file, &size) || size.u.HighPart) diff --git a/dlls/gdiplus/tests/font.c b/dlls/gdiplus/tests/font.c index 7fbe619cd93..a2aea904bb1 100644 --- a/dlls/gdiplus/tests/font.c +++ b/dlls/gdiplus/tests/font.c @@ -91,7 +91,7 @@ static void test_long_name(void) ok(file != INVALID_HANDLE_VALUE, "CreateFileW failed: %ld\n", GetLastError()); stat = GdipPrivateAddFontFile(fonts, path); - todo_wine ok(stat == Ok, "GdipPrivateAddFontFile failed with open file handle: %d\n", stat); + ok(stat == Ok, "GdipPrivateAddFontFile failed with open file handle: %d\n", stat); CloseHandle(file); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2302
On Tue Feb 28 19:18:32 2023 +0000, w-flo wrote:
I can give that a try, yes. (First open the file in the test code, then call the font loading function on the same file.) I noticed the "test_long_name()" test already tests a few different things related to GdipPrivateAddFontFile(), so I modified the test a bit to trigger the bug. I could also extract/copy that part of it to its own test function if that is preferred.
I added a "if GdipPrivateAddFontFile() fails (probably due to the sharing violation), try again after closing the file handle" so the other tests in that function continue to work on older wine versions. I'm not sure if that is a good thing or not. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2302#note_25856
I added a "if GdipPrivateAddFontFile() fails (probably due to the sharing violation), try again after closing the file handle" so the other tests in that function continue to work on older wine versions. I'm not sure if that is a good thing or not.
I think that's unnecessary; we don't and can't account for wine tests being run with older wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2302#note_25896
On Wed Mar 1 20:30:49 2023 +0000, Zebediah Figura wrote:
I added a "if GdipPrivateAddFontFile() fails (probably due to the sharing violation), try again after closing the file handle" so the other tests in that function continue to work on older wine versions. I'm not sure if that is a good thing or not. I think that's unnecessary; we don't and can't account for wine tests being run with older wine. In this case, it's necessary to account for Wine failing because the test is added before Wine is fixed. In theory, the commits could be combined to avoid this, but I think it's fine as is.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2302#note_25928
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2302
On Wed Mar 1 21:45:39 2023 +0000, Esme Povirk wrote:
In this case, it's necessary to account for Wine failing because the test is added before Wine is fixed. In theory, the commits could be combined to avoid this, but I think it's fine as is. Thank you, Zebediah and Esme! I was struggling a bit, trying to find a way to handle this. I was under the impression that we should split test + fix into separate commits, but that's probably for more complicated fixes. :smile:
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2302#note_25931
On Wed Mar 1 21:54:04 2023 +0000, w-flo wrote:
Thank you, Zebediah and Esme! I was struggling a bit, trying to find a way to handle this. I was under the impression that we should split test + fix into separate commits, but that's probably for more complicated fixes. :smile: Ah, right, I didn't notice that. But then surely the workaround could be removed in the second patch?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2302#note_25946
On Wed Mar 1 21:58:36 2023 +0000, Zebediah Figura wrote:
Ah, right, I didn't notice that. But then surely the workaround could be removed in the second patch? It could, but given that it's already done I don't see the point.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2302#note_25950
participants (4)
-
Esme Povirk (＠madewokherd) -
Florian Will
-
w-flo (＠w-flo)
-
Zebediah Figura (＠zfigura)