[PATCH] gdi32: Collapse relative paths inside true type fonts file names in registry.

[Paul Gofman]

Signed-off-by: Paul Gofman 
---
    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));

[Nikolay Sivov]

On 10/7/19 2:19 PM, Paul Gofman wrote:
> Signed-off-by: Paul Gofman
>  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?

[Paul Gofman]

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 "\..\".

[Huw Davies]

On Mon, Oct 07, 2019 at 02:19:18PM +0300, Paul Gofman wrote:
> Signed-off-by: Paul Gofman
> 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.