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
