RFR: 8337408: Use GetTempPath2 API instead of GetTempPath
Use the GetTempPath2 APIs instead of the GetTempPath APIs in native code across the OpenJDK repository to retrieve the temporary directory path, as GetTempPath2 provides enhanced security. While GetTempPath may still function without errors, using GetTempPath2 reduces the risk of potential exploits for users. The code to dynamically load GetTempPath2 is duplicated due to the following reasons. I would appreciate any suggestions to remove the duplication where possible: 1. The changes span across four different folders—java.base, jdk.package, jdk.attach, and hotspot—with no shared code between them. 2. Some parts of the code use version A, while others use version W (ANSI vs. Unicode). 3. Some parts of the code are written in C others in C++. - Commit messages: - fix spaces - Use GetTempPath2 API instead of GetTempPath Changes: https://git.openjdk.org/jdk/pull/20600/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20600&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337408 Stats: 78 lines in 4 files changed: 70 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/20600.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20600/head:pull/20600 PR: https://git.openjdk.org/jdk/pull/20600
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
> Use the GetTempPath2 APIs instead of the GetTempPath APIs in native code > across the OpenJDK repository to retrieve the temporary directory path, as > GetTempPath2 provides enhanced security. While GetTempPath may still function > without errors, using GetTempPath2 reduces the risk of potential exploits for > users. > > > The code to dynamically load GetTempPath2 is duplicated due to the following > reasons. I would appreciate any suggestions to remove the duplication where > possible: > > 1. The changes span across four different folders—java.base, jdk.package, > jdk.attach, and hotspot—with no shared code between them. > 2. Some parts of the code use version A, while others use version W (ANSI vs. > Unicode). > 3. Some parts of the code are written in C others in C++. Dhamoder Nalla has updated the pull request incrementally with one additional commit since the last revision: fix missing code - Changes: - all: https://git.openjdk.org/jdk/pull/20600/files - new: https://git.openjdk.org/jdk/pull/20600/files/6969e669..32fce98e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20600&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20600&range=00-01 Stats: 11 lines in 1 file changed: 8 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20600.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20600/head:pull/20600 PR: https://git.openjdk.org/jdk/pull/20600
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Thu, 15 Aug 2024 18:32:29 GMT, Chris Plummer wrote:
>> Dhamoder Nalla has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fix missing code
>
> src/hotspot/os/windows/os_windows.cpp line 1522:
>
>> 1520: const char* os::get_temp_directory() {
>> 1521: static char path_buf[MAX_PATH];
>> 1522: if (_GetTempPath2A != nullptr) {
>
> Where does _GetTempPath2A get initialized?
Thanks @plummercj for reviewing this PR. Fixed the missing initialization.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20600#discussion_r1719058178
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Tue, 20 Aug 2024 16:17:24 GMT, Alan Bateman wrote: >> Dhamoder Nalla has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fix missing code > > src/java.base/windows/native/libjava/java_props_md.c line 327: > >> 325: typedef DWORD (WINAPI *GetTempPath2WFnPtr)(DWORD, LPWSTR); >> 326: static GetTempPath2WFnPtr _GetTempPath2W = NULL; >> 327: static BOOL _GetTempPath2WInitialized = FALSE; > > GetJavaProperties should only be used once so I don't think you need to cache > it. > > Also I'm wondering if we can link to the function rather than using > GetProcAddress. It looks like GetTempPath2 was added in Windows 8 + Windows > Server 2012. I wonder if there is anyone building main line to older SDKs or > Windows releases where linking to GetTempPath2 would fail. Thanks @AlanBateman for reviewing this PR. GetTempPath2 is available in Windows10 Build 20348 and above as per https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppath2w#requirements - PR Review Comment: https://git.openjdk.org/jdk/pull/20600#discussion_r1723963431
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Wed, 21 Aug 2024 09:07:21 GMT, Kevin Walls wrote: > Hi, > > From the linked doc: "When calling this function from a process running as > SYSTEM it will return the path C:\Windows\SystemTemp, which is inaccessible > to non-SYSTEM processes. For non-SYSTEM processes, GetTempPath2 will behave > the same as GetTempPath." > > If SYSTEM and regular user processes have a different temp path, and we > change temp as it's known to the attach api, they will have a different > hsperfdata_username locations. > > Then does the attach api only find processes that are of the same category > (SYSTEM vs non-SYSTEM)? e.g. "jps" for a user does not show SYSTEM processes? > > SYSTEM is not the "Administrator" user, so it's not going to be a very common > problem, but if a Java process run as a SYSTEM then it could be an issue (is > that possible?). > > And if Java can't be run as a SYSTEM task, then the doc states GetTempPath2 > will behave the same as GetTempPath. Thanks @kevinjwalls for reviewing this PR The behavior has not changed; the old API GetTempPath also returns different temporary paths for SYSTEM and non-SYSTEM accounts, similar to the new API GetTempPath2. For SYSTEM accounts: GetTempPath: C:\WINDOWS\TEMP\ GetTempPath2: C:\WINDOWS\SystemTemp\ For non-SYSTEM accounts: GetTempPath: C:\Users\\\AppData\Local\Temp\ GetTempPath2: C:\Users\\\AppData\Local\Temp\ - PR Comment: https://git.openjdk.org/jdk/pull/20600#issuecomment-2329598028
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Wed, 11 Sep 2024 08:48:42 GMT, Kevin Walls wrote: > OK thanks, so the change only affects SYSTEM accounts, and such accounts > already see a different temp path to non-SYSTEM accounts. > > Newer and older Java versions run by a SYSTEM account will have different > temp paths, therefore the hsperfdata_username will be in a different place. > > (I was being picky about attach, but it's a universal thing which is expected > to work across different Java versions.) > > Do we have any apps commonly run as a Windows SYSTEM account which expect to > attach to other Java apps run by a different Java version? You're suggesting > that will have no impact and agreed it would seem really unusual. 8-) Good Question @kevinjwalls. We are not aware of any customers using attach api tools with Java application of different versions. In these cases, we recommend upgrading to the latest version. - PR Comment: https://git.openjdk.org/jdk/pull/20600#issuecomment-2356624922
Withdrawn: 8337408: Use GetTempPath2 API instead of GetTempPath
On Thu, 15 Aug 2024 16:23:18 GMT, Dhamoder Nalla wrote: > Use the GetTempPath2 APIs instead of the GetTempPath APIs in native code > across the OpenJDK repository to retrieve the temporary directory path, as > GetTempPath2 provides enhanced security. While GetTempPath may still function > without errors, using GetTempPath2 reduces the risk of potential exploits for > users. > > > The code to dynamically load GetTempPath2 is duplicated due to the following > reasons. I would appreciate any suggestions to remove the duplication where > possible: > > 1. The changes span across four different folders—java.base, jdk.package, > jdk.attach, and hotspot—with no shared code between them. > 2. Some parts of the code use version A, while others use version W (ANSI vs. > Unicode). > 3. Some parts of the code are written in C others in C++. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/20600
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Fri, 20 Sep 2024 20:35:34 GMT, Chris Plummer wrote: > > > Do we have any apps commonly run as a Windows SYSTEM account which expect > > > to attach to other Java apps run by a different Java version? You're > > > suggesting that will have no impact and agreed it would seem really > > > unusual. 8-) > > > > > > Good Question @kevinjwalls. We are not aware of any customers using attach > > api tools with Java application of different versions. In these cases, we > > recommend upgrading to the latest version. > > It has been a goal to maintain attach compatibility between different > versions of the attaching and attached to JVMs. We've fixed bugs in this area > in the past. For example, there have been changes that caused old versions of > attaching tools like jstack and jcmd to fail when attaching to newer JVMs. > We've gone back and fixed these issues in the past. Note in these cases the > issue had to do with the protocol used to communicate arguments, not the > actual attach itself. I think in the case of this PR we are talking about a > failure locate/identify the target JVM. Thank you @plummercj for sharing your insights. Could you please provide some guidance on how we can maintain compatibility? - PR Comment: https://git.openjdk.org/jdk/pull/20600#issuecomment-2375781969
Re: RFR: 8337408: Use GetTempPath2 API instead of GetTempPath [v2]
On Thu, 26 Sep 2024 16:17:49 GMT, Chris Plummer wrote: >> Dhamoder Nalla has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fix missing code > > I don't have a suggestion for maintaining compatibility other than not making > the change. Thank you @plummercj, @kevinjwalls, We appreciate your concerns regarding compatibility. However, we implemented this change to enhance security. According to the documentation at https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha#remarks, it is recommended to use GetTempPath2. If you believe that compatibility is more critical than the security improvements we've made, we are willing to close the pull request. - PR Comment: https://git.openjdk.org/jdk/pull/20600#issuecomment-2390368801
