Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request]: Make it easier for overriding ToolTask temp rsp location #9229

Closed
hknielsen opened this issue Sep 19, 2023 · 8 comments · Fixed by #9232
Closed

[Feature Request]: Make it easier for overriding ToolTask temp rsp location #9229

hknielsen opened this issue Sep 19, 2023 · 8 comments · Fixed by #9232
Labels
backlog bug Feature Request Localization Priority:2 Work that is important, but not critical for the release triaged

Comments

@hknielsen
Copy link
Contributor

hknielsen commented Sep 19, 2023

Summary

A recent issue for us that we stumbled on was a Task, inheriting from ToolTask that didnt handle Unicode chars.
The User Name contained Unitycode chars, and ToolTask puts command line args into a temp response file
https://github.com/dotnet/msbuild/blob/main/src/Utilities/ToolTask.cs#L564C61-L564C61

The issue was reported to the owners of the Task, but trying to find a way to unblock is quite hard.
The rsp path are set as: Path.Combine(Path.GetTempPath(), $"MSBuildTemp{Environment.UserName}");
GetTempPath can be overriden by changing the TMP/TEMP User env vars.
But UserName are no way to override AFAICT today.

Background and Motivation

As a consumer of a NuGet package, containing Task with such a problem, I would like to be able to not be stuck with this issue again, without being able to fix it myself.

Proposed Feature

Possible solutions on the top of my head:

  • Property on the base class to set the tmp location, so we could override the Target and send in the extra tmp location Task parameter.
  • Env var for $"MSBuildTemp{Environment.UserName}")

Edit:

  • Remove appended username on Windows
  • Change to use SecurityIdentifier

Alternative Designs

No response

@hknielsen hknielsen added Feature Request needs-triage Have yet to determine what bucket this goes in. labels Sep 19, 2023
@KalleOlaviNiemitalo
Copy link

I guess this problem only occurs on Windows, as POSIX doesn't have separate ANSI and Unicode functions for opening files.

On Windows though, the temp directory is per user. So MSBuild could be changed to omit the user name from the path entirely, when running on Windows. This wouldn't need any new environment variables.

@hknielsen
Copy link
Contributor Author

@KalleOlaviNiemitalo I like that. Took a look why the username was appended, it dosnt seem theres a clear reason:
7f3a30c#diff-3ecd811e3c6e9fd00cc8dd066953e6b280bedb594223203d0bb7786a46ca18d5L27

And before it was not, just dumping the file in the tmp location.
So changing this does not seem too harmful.

@KalleOlaviNiemitalo
Copy link

If there is some security objection to omitting the user name on Windows, then an alternative would be to use the SecurityIdentifier instead; it is as unique as the user name, and its ToString() contains ASCII characters only. That might require referencing additional libraries, though.

@hknielsen
Copy link
Contributor Author

The only reason I could see there being a security objection, would be if multiple users used the same tmp location.
Using the SecurityIdentifier would fix that. Lets use that one

@hknielsen
Copy link
Contributor Author

@Forgind Since you did the change, what do you think?
Two options:

  • Remove the appended UserName to MSBuildTemp
  • Use SecurityIdentifier instead of UserName

@AR-May AR-May added bug backlog Priority:2 Work that is important, but not critical for the release Localization and removed needs-triage Have yet to determine what bucket this goes in. labels Sep 19, 2023
hknielsen added a commit to hknielsen/msbuild that referenced this issue Sep 19, 2023
@danmoseley
Copy link
Member

I agree, I don't think this is necessary on Windows

string basePath = Path.Combine(Path.GetTempPath(), $"MSBuildTemp{Environment.UserName}");

Generally the user name is already in the Windows temp path, and if it isn't -- you're hopefully sending it some other secure place. Should that line be moved down in the non Windows path only?

@Forgind
Copy link
Member

Forgind commented Sep 19, 2023

I think removing the UserName on Windows is probably ok. The reason I initially kept it for Windows even though it was unnecessary for the security change was so that if users are looking for their MSBuild-specific temp directory, we can give them clear guidance instead of having guidance that depends on your OS.

I can ask someone internally what he thinks—I don't officially work on MSBuild anymore.

@hknielsen
Copy link
Contributor Author

Thanks for clarifying that @Forgind - that was also my thoughts, so im happy that got validated.
I remove the UserName appended to the folder here
#9232

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog bug Feature Request Localization Priority:2 Work that is important, but not critical for the release triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants