RFR 9: 8081567 : java/lang/ProcessHandle/InfoTest.java failed Cannot run program "whoami"

[Roger Riggs]

Hi Ivan,

Corrections updated in place:
     http://cr.openjdk.java.net/~rriggs/webrev-whoami-8081567/


On 6/3/2015 5:48 AM, Ivan Gerasimov wrote:
>
>
> On 03.06.2015 1:22, Roger Riggs wrote:
>> Hi Ivan,
>>
>> Thanks for the review.
>>
>> Updated the webrev in place with 2 corrections.
>>     http://cr.openjdk.java.net/~rriggs/webrev-whoami-8081567
>>
>> On 6/2/2015 5:37 PM, Ivan Gerasimov wrote:
>>> 1)
>>>
>>> +    if (domainLen > 0) {
>>> +      wcscat(domain, L"\\");
>>> +    } else {
>>> +      domain[0] = L'\0';
>>> +    }
>>> +    wcscat(domain, name);
>>>
>>> Here, domainLen is either equal to whatever LookupAccountSid has set 
>>> it to.
>>>
>>> In MSDN [1] it's written:
>>> On input, specifies the size, in TCHARs, of the 
>>> lpReferencedDomainName buffer. If the function fails because the 
>>> buffer is too small or if cchReferencedDomainName is zero, 
>>> cchReferencedDomainName receives the required buffer size, including 
>>> the terminating null character.
>>>
>>> I see in the comments that in fact on success this parameter 
>>> receives the length of the resulting string.
>>> But should we rely on that undocumented behavior?
>> Returning the actual size is (one) convention of a number of Win APIs.
>
> My concern is that I don't see this convention explicitly stated, at 
> least in the documentation for LookupAccountSidW().
> On the other hand, the doc does promise that the string will be 
> null-terminated upon successful call.
>
> Thus, I think it's safer to just remove lines 423-427 and do 
> concatenation right away, unless you have evidence that 
> LookupAccountSidW may fail to produce null-terminated string, 
> contradicting the documentation.
> In the later case, I think, it should be stated in the comments.
ok; and similar to code in WindowsNativeDispatcher.c and 
WindowsUserPrincipals
the concatenation should be unconditional.

>
>>> Does it really happen that LookupAccountSidW() returns 0, setting 
>>> domainLen to 0 and leaving the domain buffer uninitialized?
>>> From the published example [2] it seems that this shouldn't happen.
>> Only successful calls reach the code in question.
>> If domainLen is returned as zero, the domain[0] is zero terminated; 
>> not counting on the API to do it.
>> Otherwise, it returned (some) string and it is null terminated.
>> Am I misunderstanding the case?
>>>
>>> 2)
>>> I think domainLen should be set to
>>>      DWORD domainLen = sizeof(domain) - sizeof(name) - 1;
>>> Otherwise LookupAccountSid is allowed to fill up the whole buffer, 
>>> so that 'name' could not be appended.
>> Agree, but I think it should be -2;  one for the delimiter and for 
>> the trailing \0.
>
> I was thinking of 256 (== sizeof(domain) - sizeof(name) - 1) for the 
> domain + \0.
> Then the trailing \0 is replaced with the delimiter and the remaining 
> 256 WCHARs can be used for the name + \0;
ok
>
>> I could not find a maximum length of a domain or name and picked 255.
>
> The example from MSDN [2] uses 256 for the name + terminating null, so 
> I think it should be fine.
> To be extra-safe we could check GetLastError() for ERROR_NONE_MAPPED 
> and in such rare cases allocate the buffers dynamically.
I don't see that NONE_MAPPED is associated with the output buffers.

But taking the lead from the code in WindowsNativeDispatcher.c for 
producting
the SID, I added code to fallback to the String version.

Thanks, Roger

>
> Sincerely yours,
> Ivan
>
>>>
>>> 3)
>>> Indentation isn't consistent: it should be multiple of 4.
>> Thanks, helpful but not correct emacs c-mode.
>>
>> Roger
>>
>>>
>>> Sincerely yours,
>>> Ivan
>>>
>>> [1] 
>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa379166%28v=vs.85%29.aspx
>>> [2] 
>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa379554(v=vs.85).aspx
>>>
>>>> Issue:
>>>>   https://bugs.openjdk.java.net/browse/JDK-8081567
>>>>
>>>> Thanks, Roger
>>>>
>>>>
>>>
>>
>>
>