RFR: 8305667: Some fonts installed in user directory are not detected on Windows

[Phil Race]

On Wed, 5 Apr 2023 17:03:45 GMT, Nikita Gubarkov <ngubarkov at openjdk.org> wrote:

> `data` array has size of `MAX_BUFFER` like `wname`, but it has a char type, so its size is twice smaller than actual path limit, because paths returned by `RegEnumValueW` use wide chars. We need to double this limit.

src/java.desktop/windows/native/libfontmanager/fontpath.c line 522:

> 520: #define MAX_DATA_BUFFER (MAX_PATH*2+2)
> 521:     const wchar_t wname[MAX_NAME_BUFFER];
> 522:     const char data[MAX_DATA_BUFFER];

Looking at this I think we need to do more here and forget using the fixed buffer sizes and use what the query returns.

Let's consider NAME and DATA one at a time.

1) NAME
First note that we call the "W" version of the function so anything that is a string will be measured in wchars, not bytes
NAME is a string so the value we get back is the number of chars in the string.
Since we allocate using wchar_t we are OK there.
But we also need to note that the returned len doesn't include the terminating null.
So it seems to me that we need to adjust the check
dwMaxValueNameLen >= MAX_NAME_BUFFER


However going back to NAME the maximum name len has nothing to do with file paths.
So FILENAME_MAX and MAX_PATH are completely irrelevant and the code shouldn't be using it for NAME.
If there's a TTC with 10 fonts in it the name will look like
"NAME_XXXXXXXXXXXXXX_1 & NAME_XXXXXXXXXXX_2 & ... <elided> ...... & NAME_XXXXXXXXXXXX10 ... (TrueType)"
then we can exceed FILENAME_MAX and MAX_PATH very easily.
[BTW I checked and FILENAME_MAX and MAX_PATH are both 260]

So I think we need to dynamically allocate the buffer based on the returned value of dwMaxValueNameLen
which does NOT include the terminating NULL.

So I'd want code like
wname = (wchar_t*)(calloc(dwMaxValueNameLen+1, sizeof(wchar_t))

and then remember to  add code to free wname (!)

2) DATA
Since FILENAME_MAX and MAX_PATH are both 260, the current proposal doesn't change anything for NAME.
However it doubles the allocation for DATA. I can see why this is needed.
I see that for user installed fonts the registry value is the whole path (folder path + filename) ,
whereas for the installed fonts for which this was "designed" it is just the file name.
But why use the hard coded value when the registry call already told us what we need ?

RegQueryInfoKeyW specifies that it returns the data length in BYTES.
However we know its a unicode string, so depending how we allocate the array
its either len or len/2 - if its wchar_t.

wchar_t *data = (wchar_t*)(calloc(dwMaxValueNameLen, sizeof(char))

- we already cast to LPWSTR so why not make it a wchar_t anyway ?
- I used calloc just to be paranoid to get zeroed out memory
- I tested and can confirm that for DATA the returned len includes the allocation for the terminating null.

- remember to free this too
- With this dynamic allocation we no longer need the checks on lines 544/545

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13359#discussion_r1177067717