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

[Ivan Gerasimov]

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:
>> Hi Roger!
>>
>> On 02.06.2015 0:38, Roger Riggs wrote:
>>> Please review a change to report the user/owner in 
>>> ProcessHandle.info to
>>> have the same form for the owner identification as provided by 
>>> java.nio.Files.
>>> On Windows it includes the domain with the user name.
>>>
>>> The test is updated to remove a dependency on the OS command whoami
>>> and instead compare the users process with the owner of a file created
>>> by the process improving portability.
>>>
>>> Webrev:
>>>     http://cr.openjdk.java.net/~rriggs/webrev-whoami-8081567/
>>>
>>
>> 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.

>> 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;

> 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.

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
>>>
>>>
>>
>
>