Some issues on identifying user.name on Windows
Hi, I noticed following code openjdk-7-ea-src-b127-27_jan_2011\jdk\src\windows\native\java\lang\java_props_md.c /* * User name * We try to avoid calling GetUserName as it turns out to * be surprisingly expensive on NT. It pulls in an extra * 100 K of footprint. */ { WCHAR *uname = _wgetenv(L"USERNAME"); if (uname != NULL && wcslen(uname) > 0) { sprops.user_name = _wcsdup(uname); } else { WCHAR buf[100]; int buflen = sizeof(buf); sprops.user_name = GetUserNameW(buf, &buflen) ? _wcsdup(buf) : L"unknown"; } } In my opinion there are some issues in it. 1. We identify buffer length in bytes but GetUserNameW expects size in wchars. This can lead to buffer overflow. 2. In normal cases user name is allocated in heap (_wcsdup does this), but in problematic case user_name points to the static string "unknown". This can lead to problems once we decide to free memory. 3. Solaris and Linux return "?" in problematic case, but Windows returns "unknown". This seems inconsistent. I believe that Windows should also return "?" in order to make it consistent with Linux and Solaris and to let know whether username is definitely unknown. It's impossible to create user with name "?" because it has a forbidden character. On the other hand it is possible have user with name "unknown" literally, so if java says that user name is "unknown" we can't definitely know whether user is really unknown or he has such a name. I propose following code instead: /* * User name * We try to avoid calling GetUserName as it turns out to * be surprisingly expensive on NT. It pulls in an extra * 100 K of footprint. */ { WCHAR *uname = _wgetenv(L"USERNAME"); if (uname != NULL && wcslen(uname) > 0) { sprops.user_name = _wcsdup(uname); } else { sprops.user_name = IdentifyUserName(); } } static LPWSTR IdentifyUserName() { DWORD buflen = 0; GetUserNameW(0, &buflen); // identify buffer length if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { // 'buflen' receives the required buffer size in WCHARs, including the terminating null character. LPWSTR result = (LPWSTR) malloc(buflen * sizeof(WCHAR)); if (result) { if (GetUserNameW(result, &buflen)) { return result; } else { free(result); } } } return _wcsdup(L"?"); // allocate username in heap, use impossible username } Regards, Dmytro
Dmytro Sheyko wrote:
Hi,
I noticed following code
openjdk-7-ea-src-b127-27_jan_2011\jdk\src\windows\native\java\lang\java_props_md.c
/* * User name * We try to avoid calling GetUserName as it turns out to * be surprisingly expensive on NT. It pulls in an extra * 100 K of footprint. */ { WCHAR *uname = _wgetenv(L"USERNAME"); if (uname != NULL && wcslen(uname) > 0) { sprops.user_name = _wcsdup(uname); } else { WCHAR buf[100]; int buflen = sizeof(buf); sprops.user_name = GetUserNameW(buf, &buflen) ? _wcsdup(buf) : L"unknown"; } } Dmytro - I agree this code should be fixed. On the buffer size, my guess is that it's rare to have a 50+ character username and so it probably hasn't been an issue.
I think the issue of what to do when the username cannot be determined is a good question and should be re-examined. Defaulting to "?" or "unknown" is problematic as some applications need to use the value of the user.name propety in file names or as a key. We've had a few reports of problems on Linux where user.name is defaulting to "?". In the examples that I looked at it was because the 32-bit JDK was being used on a 64-bit system and the 32-bit libnss_ldap wasn't installed. Minimally we need to make these types of issues easier to diagnose, and perhaps look at the implications of having the startup fail if the username cannot be determined. In the your patch you propose to standardize on "?" if the username cannot be determined and I just wonder if there are any genuine conditions where it might fail on Windows and whether is there might be any existing code depending on "unknown" (I hope not). -Alan.
Hi, Maybe it makes sense to use environment variables on Linux (and Solaris) as well in order to minimize failure of "user.name" and "user.home" detection, doesn't it? As for Windows, many Win32 functions fail when system is shutting down, and GetUserName shouldn't be an exception. But I don't know other conditions till now. I believe that JVM shouldn't stop working if user name can't be determined because many applications do not need to know it at all. Actually system properties don't seems very reliable source for those applications that DO need user name. One can easily cheat them by setting env var USERNAME (on Windows) or just passing -Duser.name in command line explicitly. I think this kind of application needs separate API that provides true system information and better diagnostics in problematic cases. As for default value for "user.name" I changed my mind. I think we shouldn't place "user.name" to system properties at all if user name cannot be determined. (Of course, documenting this behavior well in System.getProperties() javadoc) It would be easier to handle quite rare problematic cases with following code String username = System.getProperty("user.name", fallbackUsernameValue); than with String username = System.getProperty("user.name"); if (username.equals("?") || username.equals("unknown")) { username = fallbackUsernameValue; } Regards, Dmytro Date: Tue, 8 Feb 2011 13:30:19 +0000 From: Alan.Bateman@oracle.com To: dmytro_sheyko@hotmail.com CC: core-libs-dev@openjdk.java.net Subject: Re: Some issues on identifying user.name on Windows Message body Dmytro Sheyko wrote: Hi, I noticed following code openjdk-7-ea-src-b127-27_jan_2011\jdk\src\windows\native\java\lang\java_props_md.c /* * User name * We try to avoid calling GetUserName as it turns out to * be surprisingly expensive on NT. It pulls in an extra * 100 K of footprint. */ { WCHAR *uname = _wgetenv(L"USERNAME"); if (uname != NULL && wcslen(uname) > 0) { sprops.user_name = _wcsdup(uname); } else { WCHAR buf[100]; int buflen = sizeof(buf); sprops.user_name = GetUserNameW(buf, &buflen) ? _wcsdup(buf) : L"unknown"; } } Dmytro - I agree this code should be fixed. On the buffer size, my guess is that it's rare to have a 50+ character username and so it probably hasn't been an issue. I think the issue of what to do when the username cannot be determined is a good question and should be re-examined. Defaulting to "?" or "unknown" is problematic as some applications need to use the value of the user.name propety in file names or as a key. We've had a few reports of problems on Linux where user.name is defaulting to "?". In the examples that I looked at it was because the 32-bit JDK was being used on a 64-bit system and the 32-bit libnss_ldap wasn't installed. Minimally we need to make these types of issues easier to diagnose, and perhaps look at the implications of having the startup fail if the username cannot be determined. In the your patch you propose to standardize on "?" if the username cannot be determined and I just wonder if there are any genuine conditions where it might fail on Windows and whether is there might be any existing code depending on "unknown" (I hope not). -Alan.
Dmytro Sheyko wrote:
Hi,
Maybe it makes sense to use environment variables on Linux (and Solaris) as well in order to minimize failure of "user.name" and "user.home" detection, doesn't it? Yes, I think this makes sense. Typically USER and LOGNAME are set (we might just need to think about the sudo case).
As for Windows, many Win32 functions fail when system is shutting down, and GetUserName shouldn't be an exception. But I don't know other conditions till now.
I believe that JVM shouldn't stop working if user name can't be determined because many applications do not need to know it at all.
The counter argument is that applications that do require it could fail in unpredictable ways. I've seen a couple of cases but never Windows. Another suggestion is to emit a warning, something that might be feasible for debug/fastdebug builds (probably not product because messages coming from the runtime can cause other problems, and can confuse tests). Minimally we need something to make it easier to diagnose.
Actually system properties don't seems very reliable source for those applications that DO need user name. One can easily cheat them by setting env var USERNAME (on Windows) or just passing -Duser.name in command line explicitly. I think this kind of application needs separate API that provides true system information and better diagnostics in problematic cases.
As for default value for "user.name" I changed my mind. I think we shouldn't place "user.name" to system properties at all if user name cannot be determined. (Of course, documenting this behavior well in System.getProperties() javadoc) I don't think this is an option because it would break existing code that assumes user.name is set.
It would be easier to handle quite rare problematic cases with following code
String username = System.getProperty("user.name", fallbackUsernameValue);
than with
String username = System.getProperty("user.name"); if (username.equals("?") || username.equals("unknown")) { username = fallbackUsernameValue; } We don't document "?" or "unknown" but it would not surprise me to find code like this.
Anyway back to your original patch, what would you think about modifying it so that it throws an exception if GetUserNameW fails rather than defaulting to "unknown"? You mentioned the "Windows is shutting down" case (and you may be right on that) but I can't think of any others except period insufficient resources in which case we'll probably keel over anyway. We can separate this from the Solaris/Linux changes as that is the most likely places where it might fail and default to "?" today. -Alan
Hi Alan, I am still convinced that inability to get user name is not a good reason to terminate execution (with exception or fatal error). I am afraid that this could break existing programs that perform well when user name is not known. They can be even unconscious that they work in such strange conditions. Looking at code near I can see that there is not consistent approach. Solaris/Linux part fails with exception if current directory is not known, while Windows part is optimistic about this. Maybe the best approach is somewhere between these extreme ones. Just an idea: what about to put diagnostic message just in system property? So it may look something like user.name="?: GetUserName failed, GetLastError=1115: A system shutdown is in progress." This way we provide better diagnostics than if we report plain "unknown" and at the same time we let application ignore failure if it is not relevant for it. By the way, getting current directory on Windows is also slippery. In code we inform GetCurrentDirectory that our buffer is twice longer than we actually reserved. /* Current directory */ { WCHAR buf[MAX_PATH]; GetCurrentDirectoryW(sizeof(buf), buf); sprops.user_dir = _wcsdup(buf); } Again, this can be not an issue since it's not easy to create directory longer than MAX_PATH. Regards, Dmytro Date: Wed, 9 Feb 2011 12:50:04 +0000 From: Alan.Bateman@oracle.com To: dmytro_sheyko@hotmail.com CC: core-libs-dev@openjdk.java.net Subject: Re: Some issues on identifying user.name on Windows Dmytro Sheyko wrote: Hi, Maybe it makes sense to use environment variables on Linux (and Solaris) as well in order to minimize failure of "user.name" and "user.home" detection, doesn't it? Yes, I think this makes sense. Typically USER and LOGNAME are set (we might just need to think about the sudo case). As for Windows, many Win32 functions fail when system is shutting down, and GetUserName shouldn't be an exception. But I don't know other conditions till now. I believe that JVM shouldn't stop working if user name can't be determined because many applications do not need to know it at all. The counter argument is that applications that do require it could fail in unpredictable ways. I've seen a couple of cases but never Windows. Another suggestion is to emit a warning, something that might be feasible for debug/fastdebug builds (probably not product because messages coming from the runtime can cause other problems, and can confuse tests). Minimally we need something to make it easier to diagnose. Actually system properties don't seems very reliable source for those applications that DO need user name. One can easily cheat them by setting env var USERNAME (on Windows) or just passing -Duser.name in command line explicitly. I think this kind of application needs separate API that provides true system information and better diagnostics in problematic cases. As for default value for "user.name" I changed my mind. I think we shouldn't place "user.name" to system properties at all if user name cannot be determined. (Of course, documenting this behavior well in System.getProperties() javadoc) I don't think this is an option because it would break existing code that assumes user.name is set. It would be easier to handle quite rare problematic cases with following code String username = System.getProperty("user.name", fallbackUsernameValue); than with String username = System.getProperty("user.name"); if (username.equals("?") || username.equals("unknown")) { username = fallbackUsernameValue; } We don't document "?" or "unknown" but it would not surprise me to find code like this. Anyway back to your original patch, what would you think about modifying it so that it throws an exception if GetUserNameW fails rather than defaulting to "unknown"? You mentioned the "Windows is shutting down" case (and you may be right on that) but I can't think of any others except period insufficient resources in which case we'll probably keel over anyway. We can separate this from the Solaris/Linux changes as that is the most likely places where it might fail and default to "?" today. -Alan
participants (2)
-
Alan Bateman
-
Dmytro Sheyko