On Tue, 25 Apr 2023 21:17:58 GMT, Phil Race <[email protected]> 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
