RFR 9/8: 8066504: GetVersionEx in java.base/windows/native/libjava/java_props_md.c might not get correct Windows version

[Roger Riggs]

Hi Ivan,

Thanks for the review and recommendation.

The webrev is updated in place to use WCHAR.

     http://cr.openjdk.java.net/~rriggs/webrev-win-ver-8066504/

On 6/18/2015 2:07 PM, Ivan Gerasimov wrote:
> Hi Roger!
>
> On 17.06.2015 17:27, Roger Riggs wrote:
>> I updated the webrev to remove the Windows 3.1 case and
>> will wait 24hrs to see if there are more comments.
>>
>
> I have a little concern.
> You're using char-agnostic versions of functions GetSystemDirectory(), 
> GetFileVersionInfoSize(), GetFileVersionInfo() with a plain char buffer.
> I think it would be better to either declare kernel32_path TCHAR[] or 
> explicitly use ANSI versions of the functions.
>
> Personally, I'd prefer if kernel32_path were WCHAR and Unicode 
> versions of the functions were used.
> I don't know if it's allowed to have non-ASCII chars in the 
> SysDirectory path, but it seems reasonable to assume it is.
I'll switch to the WCHAR version to avoid any doubt.

Thanks, Roger

>
> It would be interesting to setup Windows with non-English name of the 
> system directory and see what's starting to fail :)
> I'm going to do that as my spare time.
>
> Sincerely yours,
> Ivan
>
>
>> Thanks for the review, Roger
>>
>>
>> On 6/17/2015 9:58 AM, Alan Bateman wrote:
>>> On 17/06/2015 14:49, Roger Riggs wrote:
>>>> Hi Alan,
>>>>
>>>> Would there be any concerns about backporting to JDK 8?
>>> I don't think so but it will require the jdk8u maintainers to approve.
>>>>
>>>> The new helper APIs 
>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/ms724451%28v=vs.85%29.aspx> 
>>>> only verify that the OS is at least the version requested.
>>>> It does not reveal what version the OS is actually running on.
>>>> Adding a manifest attribute just declares the minimum version required
>>>> and is reported by GetVersionEx.
>>>> In the current case, the code uses GetVersionEx as a fallback in
>>>> case the version on kernel32.dll is not available.
>>>> That fallback could be removed since kernel32.dll must exist on all 
>>>> supported versions.
>>> Okay, I thought the helper APIs had better support than this. Maybe 
>>> the Microsoft folks that are contributing to OpenJDK might have 
>>> suggestions. In general I don't have any objections to the approach, 
>>> just thought it was a big odd that the OS forces us to look at the 
>>> version of kernel32.dll.
>>>
>>>
>>>> :
>>>>
>>>>>
>>>>> Also, is there any update needed in 
>>>>> src/windows/resource/java.manifest?
>>>> If using the version of kernel32.dll then the manifest version does 
>>>> not affect the value of os.name.
>>>> But yes, I added the Windows 10 supportedOS uid.
>>> I don't think this was in the first webrev that I saw but I see it 
>>> is there now, looks fine.
>>>
>>>
>>>>>
>>>>> In passing, GetNativeSystemInfo has been in Windows since Windows 
>>>>> XP so I don't think we need to use GetModuleHandle to get the 
>>>>> address now.
>>>> ok.
>>>>
>>>> Similarly, can the case for Windows 3.1 be removed?   (its not in 
>>>> theJDK 8 supported matrix 
>>>> <http://www.oracle.com/technetwork/java/javase/certconfig-2095354.html>)
>>>> caseVER_PLATFORM_WIN32s:
>>> I think that would be a fine cleanup.
>>>
>>> -Alan.
>>
>>
>>
>