Skip to content

Use GetTempPath2W if available. - #72452

Merged
adamsitnik merged 7 commits into
dotnet:mainfrom
teo-tsirpanis:gettemppath2
Aug 11, 2022
Merged

Use GetTempPath2W if available.#72452
adamsitnik merged 7 commits into
dotnet:mainfrom
teo-tsirpanis:gettemppath2

Conversation

@teo-tsirpanis

@teo-tsirpanisteo-tsirpanis commented Jul 19, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Since Windows 11 there is a new API to get the temporary files path called GetTempPath2. This API returns a directory inaccessible to non-SYSTEM processes if the calling process runs as SYSTEM, and it is recommended to call this function instead of GetTempPath.

This PR tries to find GetTempPath2W and uses that, otherwise it falls back to GetTempPathW.

I also fixed a bug where the original GetTempPathW P/Invoke was not marked with SetLastError and would fail with Operation Completed Successfuly in case of a failure.

@ghostghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Jul 19, 2022
@ghost

ghost commented Jul 19, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Since Windows 11 there is a new API to get the temporary files path called GetTempPath2. This API returns a directory inaccessible to non-SYSTEM processes if the calling process runs as SYSTEM, and it is recommended to call this function instead of GetTempPath.

This PR tries to find GetTempPath2W and uses that, otherwise it falls back to GetTempPath.

I also fixed a bug where the original GetTempPathW P/Invoke was not marked with SetLastError and would fail with Operation Completed Successfuly in case of a failure.

Author:teo-tsirpanis
Assignees:-
Labels:

area-System.IO

Milestone:-

@jkotas

jkotas commented Jul 19, 2022

Copy link
Copy Markdown
Member

Does this behavior change need to be treated as a breaking change?

jkotas
jkotas reviewed Jul 19, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated
jkotas
jkotas reviewed Jul 19, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated
stephentoub
stephentoub reviewed Jul 19, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs
@danmoseley

danmoseley commented Jul 19, 2022

Copy link
Copy Markdown
Contributor

Does this behavior change need to be treated as a breaking change?

This would be something like -- I wrote a file to temp, upgrade my .NET or OS, and try to read it? That seems unlikely to be something we want to guarantee. In general it is good to not assume that a reboot will preserve temp.

@teo-tsirpanis

teo-tsirpanis commented Jul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

And the path would change only if the program runs as SYSTEM. Most user apps are unaffected.

@danmoseley

danmoseley commented Jul 19, 2022

Copy link
Copy Markdown
Contributor

Have people tried running .NET code as SYSTEM?

@GrabYourPitchforks

GrabYourPitchforks commented Jul 19, 2022

Copy link
Copy Markdown
Member

It's not uncommon for Windows services to run as SYSTEM. It's also possible that GetTempPath2W uses the current impersonation token instead of the process's own token, which could means the answer could change while the application is running. But I don't have a good way of testing this right now.

@runfoapprunfoappBot mentioned this pull request Jul 20, 2022
Open
stephentoub
stephentoub reviewed Jul 22, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs
@teo-tsirpanis@stephentoub
Update src/libraries/System.Private.CoreLib/src/System/IO/Path.Window…
b18a9e3
…s.cs
Co-authored-by: Stephen Toub <stoub@microsoft.com>
jozkee
jozkee reviewed Aug 5, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs
teo-tsirpanisand others added 2 commits August 5, 2022 22:59
This was referenced Aug 8, 2022
Closed
Closed
@danmoseley

danmoseley commented Aug 10, 2022

Copy link
Copy Markdown
Contributor

GetGCMemoryInfo failure is already resolved.

@danmoseley

danmoseley commented Aug 10, 2022

Copy link
Copy Markdown
Contributor

@teo-tsirpanis could you please resolve the conflict so we can merge?

@teo-tsirpanis

teo-tsirpanis commented Aug 10, 2022

Copy link
Copy Markdown
ContributorAuthor

Conflicts were resolved.

@danmoseley

danmoseley commented Aug 11, 2022

Copy link
Copy Markdown
Contributor

Failure is #73679

danmoseley
danmoseley reviewed Aug 11, 2022
View reviewed changes
Comment thread...ries/System.Private.CoreLib/src/System/Runtime/InteropServices/RuntimeInformation.Windows.cs
//
// IsWow64Process2 is only available on Windows 10+, so we will perform run-time introspection via indirect load
if (NativeLibrary.TryGetExport(NativeLibrary.Load(Interop.Libraries.Kernel32), "IsWow64Process2", out IntPtr isWow64Process2Ptr))
IntPtr kernel32 = Interop.Kernel32.LoadLibraryEx(Interop.Libraries.Kernel32, 0, Interop.Kernel32.LOAD_LIBRARY_SEARCH_SYSTEM32);

@danmoseleydanmoseleyAug 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change to make load library more secure?

@jkotasjkotasAug 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes: #72452 (comment)

adamsitnik
adamsitnik approved these changes Aug 11, 2022
View reviewed changes

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution @teo-tsirpanis !

@adamsitnik

adamsitnik commented Aug 11, 2022

Copy link
Copy Markdown
Member

The failure is unrelated (#73679), merging

@adamsitnik
adamsitnik merged commit cbf2e32 into dotnet:mainAug 11, 2022
@adamsitnikadamsitnik added this to the 7.0.0 milestone Aug 11, 2022
@teo-tsirpanis
teo-tsirpanis deleted the gettemppath2 branch August 11, 2022 12:14
@DrewScogginsDrewScoggins mentioned this pull request Aug 23, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas left review comments
@jozkeejozkeejozkee left review comments
@stephentoubstephentoubstephentoub approved these changes
@adamsitnikadamsitnikadamsitnik approved these changes
+1 more reviewer
@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.IOcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@teo-tsirpanis@jkotas@danmoseley@GrabYourPitchforks@adamsitnik@stephentoub@jozkee