Allow FontInfo{,Base,Desired} to store a font name > 32 wch #3107

DHowett · 2019-10-07T20:08:18Z

Summary of the Pull Request

We now truncate the font name as it goes out to GDI APIs, in console API
servicing, and in the propsheet.

I attempted to defer truncating the font to as far up the stack as possible, so as to make FontInfo usable for the broadest set of cases.

PR Checklist

Closes Specifying a font with name longer than 32 characters causes the app to crash #602
CLA signed
Tests added/passed
Requires documentation to be updated
I'm of the corefolk

Detailed Description of the Pull Request / Additional comments

There are a couple things I have questions about, actually. I know that Settings gets memset (memsat?) by the registry deserializer, and perhaps that's another place for us to tackle. Right now, this pull request enables fonts whose names are >= 32 characters in Windows Terminal only, but the underpinnings are there for conhost as well. We'd need to explicitly break at the API, or perhaps return a failure or log something to telemetry.

Should we log truncation at the API boundary to telemetry?
Should the "launch face name" get the same treatment as the face name in Settings?
Should we fix Settings here, or later?
TrueTypeFontList is built out of things in winconp, the private console header. That fills me with dread that those structures are used for interop -- so I didn't bubble these fixes all the way up through the "default font" list, even though nominally the font list is also unbounded strings.
Is unsigned int right for codepage? For width?

I left some comments about truncating font faces around, but they could be tantamount to noise.

We now truncate the font name as it goes out to GDI APIs, in console API servicing, and in the propsheet. Fixes #602.

DHowett · 2019-10-07T20:08:31Z

I also tested with my font set to __DefaultTTFont__.

DHowett · 2019-10-07T20:09:21Z

argh, I used the wrong browser profile to submit this PR

miniksa · 2019-10-08T18:30:50Z

I think #2674 might be resolved or close to resolved with this too. Can you check that?

DHowett-MSFT · 2019-10-08T18:36:14Z

Closer, but not there yet. It's suppressed in two places, one of which still has some static wstrings (font, padding, starting directory, delimiters)

miniksa · 2019-10-08T18:46:15Z

Should we log truncation at the API boundary to telemetry?

I think so, yes.

Should the "launch face name" get the same treatment as the face name in Settings?

I think so, yes.

Should we fix Settings here, or later?

Here if it's possible. I would love for conhost to get the benefits of this too.

TrueTypeFontList is built out of things in winconp, the private console header. That fills me with dread that those structures are used for interop -- so I didn't bubble these fixes all the way up through the "default font" list, even though nominally the font list is also unbounded strings.

It's not for interop. It was just shared between the conhost and propsheet code which used to live in different depots and were both written in just plain C. Now that both of them can use C++ and live in the same depot, you're welcome to bubble it the whole way through TrueTypeFontList (and half rewrite that bugger if you like to use modern collections and stuff). Or set it as a TODO for someone else to do.

Is unsigned int right for codepage? For width?

Int might be a bit too big for codepage. I think Codepage is actually an unsigned short. But I'm fine with unsigned int if that fits better in things.

I also think font weight is 100-900 so unsigned short might be more appropriately sized there too. (Width translated to weight via asking you what "width" was all about in Teams.)

src/host/renderFontDefaults.cpp

src/interactivity/win32/menu.cpp

src/renderer/base/FontInfoBase.cpp

miniksa · 2019-10-08T18:24:06Z

src/renderer/gdi/state.cpp

@@ -396,7 +396,8 @@ GdiEngine::~GdiEngine()
        // NOTE: not using what GDI gave us because some fonts don't quite roundtrip (e.g. MS Gothic and VL Gothic)
        lf.lfPitchAndFamily = (FIXED_PITCH | FF_MODERN);

-        wcscpy_s(lf.lfFaceName, ARRAYSIZE(lf.lfFaceName), FontDesired.GetFaceName());
+        // NOTE: GDI cannot support font faces > 32 characters in length. THIS TRUNCATES THE FONT.


Do we have any idea if GDI will accept the first 31-32 characters of a font whose name is too long and still resolve the correctish one?

miniksa · 2019-10-08T18:24:43Z

src/renderer/gdi/state.cpp

@@ -396,7 +396,8 @@ GdiEngine::~GdiEngine()
        // NOTE: not using what GDI gave us because some fonts don't quite roundtrip (e.g. MS Gothic and VL Gothic)
        lf.lfPitchAndFamily = (FIXED_PITCH | FF_MODERN);

-        wcscpy_s(lf.lfFaceName, ARRAYSIZE(lf.lfFaceName), FontDesired.GetFaceName());
+        // NOTE: GDI cannot support font faces > 32 characters in length. THIS TRUNCATES THE FONT.
+        wcscpy_s(lf.lfFaceName, ARRAYSIZE(lf.lfFaceName), FontDesired.GetFaceName().data());


StringCchCopy maybe? It has an HRESULT failure code...

src/renderer/gdi/state.cpp

src/renderer/inc/FontInfoBase.hpp

src/renderer/inc/IFontDefaultList.hpp

src/host/renderFontDefaults.cpp

DHowett-MSFT

Should we log truncation at the API boundary to telemetry?

I think so, yes.

K.

Should the "launch face name" get the same treatment as the face name in Settings?

I think so, yes.

K.

Should we fix Settings here, or later?

Here if it's possible. I would love for conhost to get the benefits of this too.

This one is a far bigger workitem, so I filed #3123 as a followup. I looked into doing this to TrueTypeFontList, and it cascades up through all the dialog procedure code, so I'm scared off of doing it for this pR.

TrueTypeFontList is built out of things in winconp, the private console header. That fills me with dread that those structures are used for interop -- so I didn't bubble these fixes all the way up through the "default font" list, even though nominally the font list is also unbounded strings.

It's not for interop. It was just shared between the conhost and propsheet code which used to live in different depots and were both written in just plain C. Now that both of them can use C++ and live in the same depot, you're welcome to bubble it the whole way through TrueTypeFontList (and half rewrite that bugger if you like to use modern collections and stuff). Or set it as a TODO for someone else to do.

Roger, but as above: propsheet needs more modernization than conhost today to get this bootstrapped.

Is unsigned int right for codepage? For width?

Int might be a bit too big for codepage. I think Codepage is actually an unsigned short. But I'm fine with unsigned int if that fits better in things.

I also think font weight is 100-900 so unsigned short might be more appropriately sized there too. (Width translated to weight via asking you what "width" was all about in Teams.)

I was going to say "bah i'll stick with int", but then I decided to use short and use gsl's throwing narrower so that we can root out anybody who doesn't comply.

It looks like SetCurrentConsoleFont lets you pass an int for weight, but the docs say it ranges from 0-1000 so maybe this is fine? That one's getting a non-throwing narrow.

src/renderer/base/FontInfoBase.cpp

DHowett-MSFT · 2019-10-08T22:30:10Z

Actually, on the telemetry front: we'll get a log and fail to service the API call when the font name is too long. Right now, there's no way to get a font name that's too long... so I'm going to tag this with #3123 as well.

DHowett-MSFT · 2019-10-11T15:38:05Z

/azp run

azure-pipelines · 2019-10-11T15:38:19Z

Azure Pipelines successfully started running 1 pipeline(s).

DHowett-MSFT · 2019-10-11T18:29:49Z

Okay, clearly I regressed this.

miniksa · 2019-10-11T22:06:11Z

src/host/getset.cpp

@@ -269,7 +269,8 @@ void ApiRoutines::GetNumberOfConsoleMouseButtonsImpl(ULONG& buttons) noexcept
        consoleFontInfoEx.FontFamily = fontInfo.GetFamily();
        consoleFontInfoEx.FontWeight = fontInfo.GetWeight();

-        RETURN_IF_FAILED(StringCchCopyW(consoleFontInfoEx.FaceName, ARRAYSIZE(consoleFontInfoEx.FaceName), fontInfo.GetFaceName()));
+        // GH#3123: We will fail to service the API if the font name is longer than 32 characters.


Ehhh can we just truncate it to safely fit? I feel that's better than failing.

Maybe we want a method that's like "GetNameGdiCompatible()" that fills an LF_FACESIZE size array.

src/host/settings.cpp

miniksa · 2019-10-11T22:07:19Z

src/host/settings.cpp

@@ -404,13 +403,13 @@ void Settings::SetFilterOnPaste(const bool fFilterOnPaste)
    _fFilterOnPaste = fFilterOnPaste;
 }

-const WCHAR* const Settings::GetLaunchFaceName() const
+const std::wstring& Settings::GetLaunchFaceName() const


What? Why? Why not just a std::wstring_view?

/shrug no real reason, i went with your first suggestion instead of your second

miniksa · 2019-10-11T22:09:05Z

src/renderer/gdi/state.cpp

@@ -374,7 +374,7 @@ GdiEngine::~GdiEngine()
        // Because the API is affected by the raster/TT status of the actively selected font, we can't have
        // GDI choosing a TT font for us when we ask for Raster. We have to settle for forcing the current system
        // Terminal font to load even if it doesn't have the glyphs necessary such that the APIs continue to work fine.
-        if (0 == wcscmp(FontDesired.GetFaceName(), L"Terminal"))
+        if (FontDesired.GetFaceName() == L"Terminal")


I know it didn't have a whole lot to do with you here, but should the magic string L"Terminal" be a constexpr at the top instead of here?

miniksa

Yep, good enough. Thanks.

DHowett-MSFT · 2019-10-12T00:22:46Z

/azp run

azure-pipelines · 2019-10-12T00:22:56Z

Azure Pipelines successfully started running 1 pipeline(s).

DHowett-MSFT · 2019-10-12T22:26:45Z

I'm extremely confused/alarmed.

The selection tests were failing because the unique_ptr holding the Selection singleton at static file scope was being overwritten with 0x00000000FFFFFFFF. It then later failed the null check in Selection::Instance(), so that value was returned as the Selection instance.

Time travel tracing indicates that the constructor for CursorBlinker CONSOLE_INFORMATION::_blinker is writing to the same storage as the unique_ptr when it initializes the timer timeout to INFINITE.

Why do they share storage?

DHowett-MSFT · 2019-10-12T23:20:27Z

It turns out that there's an ODR violation! CONSOLE_INFORMATION changes size based on whether UNIT_TESTING is defined (VtIo got a new UNIT_TESTING-only member in #2525), and that gets rolled up into Host.unittest.lib. InteractivityBase.lib has a compile-time dependency on the layout of CONSOLE_INFORMATION, but InteractivityBase isn't built with a UNIT_TESTING variant. CI gets constructed in InteractivityBase, and as the linker can resolve the ODR violation in whatever way it sees fit it ends up choosing the shorter layout when packing in file-level statics.

This bug only exists in our tests. The fix in 83fcb6c moves the trampled global out of the way... but just fixes the symptom. :|
/cc @miniksa

DHowett-MSFT · 2019-10-12T23:27:42Z

Followup filed in #3170.

ghost · 2019-10-23T19:55:15Z

🎉Windows Terminal Preview v0.6.2951.0 has been released which incorporates this pull request.:tada:

Handy links:

Allow FontInfo{,Base,Desired} to store a font name > 32 wch

26d3fcb

We now truncate the font name as it goes out to GDI APIs, in console API servicing, and in the propsheet. Fixes #602.

miniksa requested changes Oct 8, 2019

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 8, 2019

DHowett-MSFT reviewed Oct 8, 2019

View reviewed changes

src/renderer/base/FontInfoBase.cpp Outdated Show resolved Hide resolved

DHowett added 2 commits October 8, 2019 15:38

Code review feedback

8ab7a7c

FORMAT C: /Q

5f7d3b5

miniksa reviewed Oct 11, 2019

View reviewed changes

DHowett added 2 commits October 11, 2019 16:03

Do more good and fun things

fdfd826

And make it a span

f4daa08

miniksa approved these changes Oct 11, 2019

View reviewed changes

Fix the crashing Selection test

83fcb6c

DHowett-MSFT force-pushed the dev/duhowett/font32 branch from ec32aca to 83fcb6c Compare October 12, 2019 19:12

DHowett-MSFT mentioned this pull request Oct 12, 2019

CONSOLE_INFORMATION has an ODR violation contributing to test instability #3170

Closed

carlos-zamora approved these changes Oct 15, 2019

View reviewed changes

DHowett-MSFT merged commit b664761 into master Oct 15, 2019

DHowett-MSFT deleted the dev/duhowett/font32 branch October 15, 2019 04:23

ghost mentioned this pull request Oct 23, 2019

Specifying a font with name longer than 32 characters causes the app to crash #602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow FontInfo{,Base,Desired} to store a font name > 32 wch #3107

Allow FontInfo{,Base,Desired} to store a font name > 32 wch #3107

DHowett commented Oct 7, 2019

DHowett commented Oct 7, 2019

DHowett commented Oct 7, 2019

miniksa commented Oct 8, 2019

DHowett-MSFT commented Oct 8, 2019

miniksa commented Oct 8, 2019

miniksa Oct 8, 2019

miniksa Oct 8, 2019

DHowett-MSFT left a comment

DHowett-MSFT commented Oct 8, 2019

DHowett-MSFT commented Oct 11, 2019

azure-pipelines bot commented Oct 11, 2019

DHowett-MSFT commented Oct 11, 2019

miniksa Oct 11, 2019

miniksa Oct 11, 2019

miniksa Oct 11, 2019

DHowett-MSFT Oct 11, 2019

miniksa Oct 11, 2019

miniksa left a comment

DHowett-MSFT commented Oct 12, 2019

azure-pipelines bot commented Oct 12, 2019

DHowett-MSFT commented Oct 12, 2019

DHowett-MSFT commented Oct 12, 2019

DHowett-MSFT commented Oct 12, 2019

ghost commented Oct 23, 2019

Allow FontInfo{,Base,Desired} to store a font name > 32 wch #3107

Allow FontInfo{,Base,Desired} to store a font name > 32 wch #3107

Conversation

DHowett commented Oct 7, 2019

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

DHowett commented Oct 7, 2019

DHowett commented Oct 7, 2019

miniksa commented Oct 8, 2019

DHowett-MSFT commented Oct 8, 2019

miniksa commented Oct 8, 2019

miniksa Oct 8, 2019

Choose a reason for hiding this comment

miniksa Oct 8, 2019

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Oct 8, 2019

DHowett-MSFT commented Oct 11, 2019

azure-pipelines bot commented Oct 11, 2019

DHowett-MSFT commented Oct 11, 2019

miniksa Oct 11, 2019

Choose a reason for hiding this comment

miniksa Oct 11, 2019

Choose a reason for hiding this comment

miniksa Oct 11, 2019

Choose a reason for hiding this comment

DHowett-MSFT Oct 11, 2019

Choose a reason for hiding this comment

miniksa Oct 11, 2019

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Oct 12, 2019

azure-pipelines bot commented Oct 12, 2019

DHowett-MSFT commented Oct 12, 2019

DHowett-MSFT commented Oct 12, 2019

DHowett-MSFT commented Oct 12, 2019

ghost commented Oct 23, 2019