[PATCH] gdi32: Collapse relative paths inside true type fonts file names in registry.
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- The motivation under this patch is fixing the crash in Halo Online which originates from libcef.dll with Wine Staging. The crash scenario is the following. A staging patch puts a 'Times New Roman' font replacement to <datadir>/fonts/times.ttf. This is the only relevant difference introduced by Staging. When the font file is found there, gdi32 uses wine_get_data_dir() for a directory prefix to construct font file name. Later libcef.dll works with fonts through dwrite, and somehow gets confused by a "\\..\\" inside the font file name which it gets with IDWriteLocalFontFileLoader_GetFilePathFromKey() from system font file enumerator object. dlls/gdi32/freetype.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c index e3cff25b76..2f21ba40a0 100644 --- a/dlls/gdi32/freetype.c +++ b/dlls/gdi32/freetype.c @@ -3168,11 +3168,33 @@ static void update_reg_entries(void) HeapFree( GetProcessHeap(), 0, buffer ); if (path) - file = path; + { + len = GetFullPathNameW(path, 0, NULL, NULL); + if (!(file = HeapAlloc(GetProcessHeap(), 0, len * sizeof(*file)))) + { + HeapFree(GetProcessHeap(), 0, path); + HeapFree(GetProcessHeap(), 0, valueW); + continue; + } + if (GetFullPathNameW(path, len, file, NULL) != len - 1) + { + ERR("Could not get full path name, path %s.\n", debugstr_w(path)); + HeapFree(GetProcessHeap(), 0, file); + HeapFree(GetProcessHeap(), 0, path); + HeapFree(GetProcessHeap(), 0, valueW); + continue; + } + HeapFree(GetProcessHeap(), 0, path); + path = file; + } else if ((file = strrchrW(face->file, '/'))) + { file++; + } else + { file = face->file; + } len = strlenW(file) + 1; RegSetValueExW(winnt_key, valueW, 0, REG_SZ, (BYTE*)file, len * sizeof(WCHAR)); -- 2.21.0
On 10/7/19 2:19 PM, Paul Gofman wrote:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- The motivation under this patch is fixing the crash in Halo Online which originates from libcef.dll with Wine Staging. The crash scenario is the following.
A staging patch puts a 'Times New Roman' font replacement to <datadir>/fonts/times.ttf. This is the only relevant difference introduced by Staging. When the font file is found there, gdi32 uses wine_get_data_dir() for a directory prefix to construct font file name. Later libcef.dll works with fonts through dwrite, and somehow gets confused by a "\\..\\" inside the font file name which it gets with IDWriteLocalFontFileLoader_GetFilePathFromKey() from system font file enumerator object.
Is it the same for tahoma.ttf?Were you able to figure out where in CEF it fails and why exactly?
On 10/7/19 15:21, Nikolay Sivov wrote:
Is it the same for tahoma.ttf?Were you able to figure out where in CEF it fails and why exactly?
It is the same for tahoma.ttf in a way that it also gets "\\..\\" in the path of course, but only times.ttf affects the crash I debugged. I didn't figure exact reason inside libcef.dll. I just can say that crash happens inside its code using some (probably class) pointers initialized earlier. The minimal fix which makes me think that the core reason is in treating the font file name outside of Wine is the fact that I can workaround the crash by using GetFullPathName for returned file name in localfontfileloader_GetFilePathFromKey() only (ddraw/font.c). As far as I can tell, this function is not used anywhere in Wine directly. This still leaves some chance of course that the output of this function was passed to some Wine function which mishandles the path with "..", but I could not find this path passed to anything with +font, +file or +relay log. The sequence of fonts queries to dwrite changes after getting "Times New Roman" from enumeration through dwrite between successful sequence (with the path without "..") and the failing one. I also don't think that breaking on a font path with ".." is likely as we would likely have this problem exhibit itself somewhere else. This leaves some small chance of course that the true core reason lies somewhere deeper. My reasoning was that avoiding relative path parts in font file name is the right thing to do regardless. I was also thinking of suggesting the fix right to dwrite/font.c create_local_file_reference(), as the documentation to IDWriteFactory_CreateFontFileReference suggests that file path must be absolute path. But it looks reasonable to me to avoid unusual path in registry filled by gdi32 also, as it also exposed to the applications. Besides, according to my testing on Windows, while _CreateFontFileReference rejects truly relative paths, it is ok with the paths containing "\\..\\".
On Mon, Oct 07, 2019 at 02:19:18PM +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- The motivation under this patch is fixing the crash in Halo Online which originates from libcef.dll with Wine Staging. The crash scenario is the following.
A staging patch puts a 'Times New Roman' font replacement to <datadir>/fonts/times.ttf. This is the only relevant difference introduced by Staging. When the font file is found there, gdi32 uses wine_get_data_dir() for a directory prefix to construct font file name. Later libcef.dll works with fonts through dwrite, and somehow gets confused by a "\\..\\" inside the font file name which it gets with IDWriteLocalFontFileLoader_GetFilePathFromKey() from system font file enumerator object.
dlls/gdi32/freetype.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c index e3cff25b76..2f21ba40a0 100644 --- a/dlls/gdi32/freetype.c +++ b/dlls/gdi32/freetype.c @@ -3168,11 +3168,33 @@ static void update_reg_entries(void) HeapFree( GetProcessHeap(), 0, buffer );
if (path) - file = path; + { + len = GetFullPathNameW(path, 0, NULL, NULL); + if (!(file = HeapAlloc(GetProcessHeap(), 0, len * sizeof(*file)))) + { + HeapFree(GetProcessHeap(), 0, path); + HeapFree(GetProcessHeap(), 0, valueW); + continue; + } + if (GetFullPathNameW(path, len, file, NULL) != len - 1) + { + ERR("Could not get full path name, path %s.\n", debugstr_w(path)); + HeapFree(GetProcessHeap(), 0, file); + HeapFree(GetProcessHeap(), 0, path); + HeapFree(GetProcessHeap(), 0, valueW); + continue; + } + HeapFree(GetProcessHeap(), 0, path); + path = file; + }
All of these error paths are clumsy. Let's add a helper: WCHAR *get_full_path_name(const WCHAR *name) Note it should also check the return value of the first GetFullPathNameW call unlike the above. It would be used instead of the above if block something like: if (path) { if (fullpath = get_full_path_name(path)) { HeapFree(GetProcessHeap(), 0, path); path = fullpath; } file = path; }
else if ((file = strrchrW(face->file, '/'))) + { file++; + } else + { file = face->file; + }
Huw.
participants (3)
-
Huw Davies -
Nikolay Sivov -
Paul Gofman