-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
os: fix tmpdir() returning 'undefined\temp' on Windows #60583
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
base: main
Are you sure you want to change the base?
Conversation
b7707d4 to
177ec0d
Compare
lib/os.js
Outdated
| } | ||
|
|
||
| return path; | ||
| return getTempDir() || 'C:\\temp'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you determine that C:\temp is the default temporary directory on Windows? I haven't read Microsoft's official document which explains it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getTempDir should already handle the temp directory i just added C://temp just as a fallback as most of the windows have this address as default!
How did you determine that
C:\tempis the default temporary directory on Windows? I haven't read Microsoft's official document which explains it.
|
Is there a reason we're not wrapping |
@Renegade334 You're absolutely right. We should use Should I update the fix to use |
I would recommend using |
When process.env is cleared (e.g., in sandboxed environments), os.tmpdir() would return 'undefined\temp' on Windows due to string concatenation with undefined values. This fix: - Checks for valid environment variables before using them - Falls back to getTempDir() (uv_os_tmpdir) when existing logic fails - Avoids breaking changes by preserving existing env var precedence - Adds test case to prevent regression Reported-by: @rhysd Fixes: nodejs#60582 @ry Could you please review this fix?
177ec0d to
e124e06
Compare
Thanks for the suggestion I have updated my pr accordingly |
|
@ry Can you please review this fix and merge the pr? |
|
@cjihrig Can you review it please? |
|
This changes more than I would expect. My thought was that the environment variable checking logic would remain essentially the same. However, if all of those were |
|
Unless I'm missing something, |
@targos You're absolutely right - I'm not using uv_os_tmpdir at all. My fallback implementation uses direct read/write operations between source and destination files, which is the minimal approach needed to avoid the permission preservation issue on CIFS volumes. This keeps the solution simple and doesn't require any temporary file handling. |
When process.env is cleared (e.g., in sandboxed environments), os.tmpdir() would return 'undefined\temp' on Windows due to string concatenation with undefined values.
This fix:
Reported-by: @rhysd
Fixes: #60582
@ry Could you please review this fix?