RFR(S) 8190737: use unicode version of the canonicalize() function to handle long path on windows

[Xueming Shen]

Hi Calvin,

I believe I was thinking of keeping the "getPrefixed" as an 
implementation details and
instead exporting a "A" version of winFileHandleOpen() back then. But I 
don't have a
strong opinion on this one, so your approach looks fine.

No, I don't have a better idea for now to avoid those mb->ws->mb as long 
as the canonicalize()
and zip_open() are two separate invocations. But since they are only for 
 > max_path path. I'm
fine with it.

Yes, I think it's time to consider to migrate from interpreting the 
char* as "mb chars" to probably
"utf8 chars" (take a step back, I don't think it's easy to do to 
actually a wchar interface) for windows
platform, but that would be a separate and big rfes.

nit:

130             fname,              /* Ascii char path name */

the comment probably should be "path name in mb char", or ANSI charset.

Thanks,

-Sherman

On 9/12/18, 4:16 PM, Calvin Cheung wrote:
> Hi Sherman,
>
> Thanks for your review.
> Please refer to my reply below...
>
> On 9/10/18, 5:05 PM, Xueming Shen wrote:
>> On 9/10/18, 11:42 AM, Calvin Cheung wrote:
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8190737
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8190737/webrev.00/
>>>
>>> Please review this change for handling long path specified in the 
>>> -Xbootclasspath/a on windows.
>>>
>>> Highlight of changes:
>>> - canonicalize_md.c
>>>     it makes use of the unicode version of canonicalize 
>>> (wcanonicalize) if the original path is >= MAX_PATH.
>> So we are converting mb->wc->mb in wcanonicalize (when > max_path), 
>> and then feed the
>> resulting path into ZFILE_Open, in which we again do mb->wc-> + 
>> prefix -> CreateFileW(...)?
> That's the minimal change I could come up with. Let me know if you 
> have other suggestions.
>>
>> Maybe it's time to consider a "wchar" interface between vm and 
>> libraries.
> Good idea. I think it should be done in a separate RFE.
>> Maybe it's fine here given we
>> are only do this for > max_path case?
> This was done so that this change has no impact on the <= MAX_PATH case.
>>
>> Alan, now reading the win version canonicalize(...), remind me what's 
>> the real purpose of doing
>> FindFirstFile/FindClose() here? the API appears to suggest it is used 
>> to find the first match if there
>> is/are wildcards but we actually have checked/rejected the wildcards 
>> at the very beginning ? To
>> get the "real name" for case?
>>
>>>
>>> - zip_util.c
>>>     it uses the unicode version of CreateFile (CreateFileW) if the 
>>> original path is >= MAX_PATH.
>>>
>>> - io_util_md.c
>>>     Since zip_util.c (libzip) calls the getPrefixed() function in 
>>> canonicalize_md.c (libjava), the getPrefixed() function needs to be 
>>> exported.
>>
>> I kinda remembered that we once wanted to avoid export an variant of 
>> winFileHandleOpen() from
>> libjava to libzip ... in this case the alternative is to simply 
>> copy/paste the getPrefix into libzip ...
>> but I don't remember the exact concern back then.
> I also thought of copy/paste the getPrefix function into libzip.
> After looking at the lib/CoreLibraries.gmk, I think libzip already has 
> a dependency on libjava:
> $(eval $(call SetupJdkLibrary, BUILD_LIBZIP, \
>     NAME := zip, \
>     OPTIMIZATION := LOW, \
>     EXCLUDES := $(LIBZIP_EXCLUDES), \
>     CFLAGS := $(CFLAGS_JDKLIB) \
>         $(LIBZ_CFLAGS), \
>     CFLAGS_unix := $(BUILD_LIBZIP_MMAP) -UDEBUG, \
>     LDFLAGS := $(LDFLAGS_JDKLIB) \
>         $(call SET_SHARED_LIBRARY_ORIGIN), \
>     LIBS_unix := -ljvm -ljava $(LIBZ_LIBS), \
>     LIBS_windows := jvm.lib $(WIN_JAVA_LIB), \
> ))
>
> I've done tier1,2,3 testing and didn't see any new failures.
> I can do higher tier testing if needed.
>
> thanks,
> Calvin
>
> p.s. I didn't see this email until Ioi forwarded it to me since I 
> wasn't in the core-libs-dev alias. (I've subscribed to the alias this 
> morning.)
>
>>
>> -Sherman
>>
>>
>>
>>>
>>>
>>> - java_md.h
>>>     The JVM_MAXPATHLEN has been increased from _MAX_PATH to 1024. It 
>>> is because the the current usage of the canonicalize() function from 
>>> vm code is to preallocate the output buffer as follows:
>>>             char* canonical_path = 
>>> NEW_RESOURCE_ARRAY_IN_THREAD(thread, char, JVM_MAXPATHLEN);
>>>             if (!get_canonical_path(path, canonical_path, 
>>> JVM_MAXPATHLEN)) {
>>>     Also the unix version of canonicalize() function has the 
>>> following check:
>>>             int
>>>            canonicalize(char *original, char *resolved, int len)
>>>            {
>>>                if (len < PATH_MAX) {
>>>                errno = EINVAL;
>>>                return -1;
>>>            }
>>>    So dynamically allocating the output buffer requires more work 
>>> beyond the scope of this change.
>>>
>>> - LongBCP.java
>>>     added a scenario to the test - a long path to a jar file in 
>>> -Xbootclasspath/a.
>>>
>>> Testing: tier-[1,2,3].
>>>
>>> thanks,
>>> Calvin
>>