RFR 9: 8081567 : java/lang/ProcessHandle/InfoTest.java failed Cannot run program "whoami"
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/ Issue: https://bugs.openjdk.java.net/browse/JDK-8081567 Thanks, Roger
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? 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. 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. 3) Indentation isn't consistent: it should be multiple of 4. Sincerely yours, Ivan [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa379166%28v=vs.85%... [2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa379554(v=vs.85).a...
Issue: https://bugs.openjdk.java.net/browse/JDK-8081567
Thanks, Roger
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. 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 could not find a maximum length of a domain or name and picked 255.
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%... [2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa379554(v=vs.85).a...
Issue: https://bugs.openjdk.java.net/browse/JDK-8081567
Thanks, Roger
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%... [2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa379554(v=vs.85).a...
Issue: https://bugs.openjdk.java.net/browse/JDK-8081567
Thanks, Roger
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%... [2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa379554(v=vs.85).a...
Issue: https://bugs.openjdk.java.net/browse/JDK-8081567
Thanks, Roger
Thanks! ------------------ test/java/lang/ProcessHandle/InfoTest.java ------------------ Looks good ------------------ src/java.base/windows/native/libjava/ProcessHandleImpl_win.c ------------------ The changes look good to me! I guess it does have more sense to use StringSID if the length of user name of domain exceeds 256 chars.
Corrections updated in place: http://cr.openjdk.java.net/~rriggs/webrev-whoami-8081567/
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.
Yes, my mistake. That's output value of domainLen and nameLen that have to be analyzed to detect 'buffer is too small' condition. But I think, what you have is fine. A couple of minor nits. I would be just a little bit happier, if it were 397 WCHAR domain[255 + 1 + 255 + 1]; // large enough to concat with '/' and name 398 WCHAR name[255 *+ 1*]; 399 DWORD domainLen = *sizeof(domain) - sizeof(name)*; Otherwise, we would never use the last char of 'domain' :-) You may want to remove the extra space in the signature of procToUser(_ ), in both the forward declaration and definition. Sincerely yours, Ivan
Hi Ivan, Thanks for the cleanup suggestions. Roger On 6/3/2015 5:58 PM, Ivan Gerasimov wrote:
Thanks!
------------------ test/java/lang/ProcessHandle/InfoTest.java ------------------ Looks good
------------------ src/java.base/windows/native/libjava/ProcessHandleImpl_win.c ------------------ The changes look good to me! I guess it does have more sense to use StringSID if the length of user name of domain exceeds 256 chars.
Corrections updated in place: http://cr.openjdk.java.net/~rriggs/webrev-whoami-8081567/
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.
Yes, my mistake. That's output value of domainLen and nameLen that have to be analyzed to detect 'buffer is too small' condition. But I think, what you have is fine.
A couple of minor nits. I would be just a little bit happier, if it were 397 WCHAR domain[255 + 1 + 255 + 1]; // large enough to concat with '/' and name 398 WCHAR name[255 *+ 1*]; 399 DWORD domainLen = *sizeof(domain) - sizeof(name)*;
Otherwise, we would never use the last char of 'domain' :-)
You may want to remove the extra space in the signature of procToUser(_ ), in both the forward declaration and definition.
Sincerely yours, Ivan
participants (2)
-
Ivan Gerasimov
-
Roger Riggs