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
