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

[Nikita Gubarkov]

On Tue, 25 Apr 2023 21:17:58 GMT, Phil Race <prr at openjdk.org> wrote:

>> Nikita Gubarkov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Use malloc instead of fixed size buffers.
>
> 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 term...

Changed to dynamic allocation as you suggested. Though I used malloc, because it seems to be used more common across JDK code.

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

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