Skip to content

Commit

Permalink
Use libuv for tempdir function (#31434)
Browse files Browse the repository at this point in the history 
Fixes the Windows tempdir function returning a path with the trailing slash.
  • Loading branch information
@musm @ararslan
musm authored and ararslan committed Jun 26, 2019
1 parent f7f482b commit 2b08860
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 20 deletions.
42 changes: 24 additions & 18 deletions base/file.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,18 +424,34 @@ function touch(path::AbstractString)
path
end

const temp_prefix = "jl_"

if Sys.iswindows()
"""
tempdir()
Gets the path of the temporary directory. On Windows, `tempdir()` uses the first environment
variable found in the ordered list `TMP`, `TEMP`, `USERPROFILE`. On all other operating
systems, `tempdir()` uses the first environment variable found in the ordered list `TMPDIR`,
`TMP`, `TEMP`, and `TEMPDIR`. If none of these are found, the path `"/tmp"` is used.
"""
function tempdir()
temppath = Vector{UInt16}(undef, 32767)
lentemppath = ccall(:GetTempPathW, stdcall, UInt32, (UInt32, Ptr{UInt16}), length(temppath), temppath)
windowserror("GetTempPath", lentemppath >= length(temppath) || lentemppath == 0)
resize!(temppath, lentemppath)
return transcode(String, temppath)
buf = Base.StringVector(AVG_PATH - 1) # space for null-terminator implied by StringVector
sz = RefValue{Csize_t}(length(buf) + 1) # total buffer size including null
while true
rc = ccall(:uv_os_tmpdir, Cint, (Ptr{UInt8}, Ptr{Csize_t}), buf, sz)
if rc == 0
resize!(buf, sz[])
return String(buf)
elseif rc == Base.UV_ENOBUFS
resize!(buf, sz[] - 1) # space for null-terminator implied by StringVector
else
uv_error(:tmpdir, rc)
end
end
end

const temp_prefix = "jl_"

if Sys.iswindows()

function _win_tempname(temppath::AbstractString, uunique::UInt32)
tempp = cwstring(temppath)
temppfx = cwstring(temp_prefix)
Expand Down Expand Up @@ -481,9 +497,6 @@ function tempname()
return s
end

# Obtain a temporary directory's path.
tempdir() = dirname(tempname())

# Create and return the name of a temporary file along with an IOStream
function mktemp(parent=tempdir())
b = joinpath(parent, temp_prefix * "XXXXXX")
Expand All @@ -496,13 +509,6 @@ end
end # os-test


"""
tempdir()
Obtain the path of a temporary directory (possibly shared with other processes).
"""
tempdir()

"""
tempname()
Expand Down
35 changes: 33 additions & 2 deletions test/file.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,39 @@ close(s)
end
end

my_tempdir = tempdir()
@test isdir(my_tempdir) == true
@testset "tempdir" begin
my_tempdir = tempdir()
@test isdir(my_tempdir)
@test my_tempdir[end] != '/'
@test my_tempdir[end] != '\\'

var = Sys.iswindows() ? "TMP" : "TMPDIR"
PATH_PREFIX = Sys.iswindows() ? "C:\\" : "/tmp/"
# Warning: On Windows uv_os_tmpdir internally calls GetTempPathW. The max string length for
# GetTempPathW is 261 (including the implied trailing backslash), not the typical length 259.
# We thus use 260 (with implied trailing slash backlash this then gives 261 chars) and
# subtract 9 to account for i = 0:9.
MAX_PATH = (Sys.iswindows() ? 260-9 : 1024) - length(PATH_PREFIX)
for i = 0:8
local tmp = PATH_PREFIX * "x"^MAX_PATH * "123456789"[1:i]

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 28, 2020

Member

@musm, why does this generate too-long tmp paths? Is that intentional or was the intention to make these exactly the max path length? This is causing problems when I test #33593 because even stating the too long path is an error and the tempdir path definitely cannot exist.

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 28, 2020

Member

In general, I don't really get what this is testing. Clearly something about tmpdir working, but why make the tempdir so long?

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 28, 2020

Author Contributor

@StefanKarpinski this is testing a boundary case with respect to the buffer length, where I discovered a bug in libuv. This is the only reason the length is so long to ensure julia is protected against this scenario. Which was fixed in libuv/libuv#2341

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 28, 2020

Member

So the intention is for tmp to have the maximum allowable path length? Because currently it's making paths longer than that and the only reason it doesn't fail is because the path is never actually created.

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 28, 2020

Author Contributor

It's failing in your PR because here we change the ENV variable TMP to arbitrary directories that haven't been created to test all the various buffer length edge cases. This is a valid test, since tempdir is simply returning the value of the environment variable (it doesn't require or check that the folder actually exist).

See http://docs.libuv.org/en/v1.x/misc.html#c.uv_os_tmpdir

And
https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppathw

Note that the function does not verify that the path exists, nor does it test to see if the current process has any kind of access rights to the path. The GetTempPath function returns the properly formatted string that specifies the fully qualified path based on the environment variable search order as previously specified. The application should verify the existence of the path and adequate access rights to the path prior to any use for file I/O operations.

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 28, 2020

Member

That's not actually why it's failing: it's failing because it cannot even stat the given path since the path is too long. If it was only failing because the path didn't exist, it would be failing with that explicit error.

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 28, 2020

Member

Specifically, it's failing because the file name is too long, even if the entire path name is short enough.

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 28, 2020

Author Contributor

Are you referring to this test specifically?

Running the changes you made in your PR

import Base.Filesystem.AVG_PATH
import Base.RefValue

function _tempdir()
    buf = Base.StringVector(AVG_PATH - 1) # space for null-terminator implied by StringVector
    sz = RefValue{Csize_t}(length(buf) + 1) # total buffer size including null
    while true
        rc = ccall(:uv_os_tmpdir, Cint, (Ptr{UInt8}, Ptr{Csize_t}), buf, sz)
        if rc == 0
            resize!(buf, sz[])
            break
        elseif rc == Base.UV_ENOBUFS
            resize!(buf, sz[] - 1)  # space for null-terminator implied by StringVector
        else
            uv_error(:tmpdir, rc)
        end
    end
    p = String(buf)
    s = stat(p)
    ispath(s) || error("tempdir path does not exist: $p")
    isdir(s) || error("tempdir path is not a directory: $p")
    return p
end


using Test



 var =  Sys.iswindows() ? "TMP" : "TMPDIR"
 PATH_PREFIX = Sys.iswindows() ? "C:\\" : "/tmp/"
 MAX_PATH = (Sys.iswindows() ? 260-9 : 1024) - length(PATH_PREFIX)


    for i = 0:8
        @show i
        local tmp = PATH_PREFIX * "x"^MAX_PATH * "123456789"[1:i]
            @test withenv(var => tmp) do
            _tempdir()
        end == (tmp)
    end

I get

julia> include("C:\\Users\\Mus\\Desktop\\testtmp.jl");
i = 0
Error During Test at C:\Users\Mus\Desktop\testtmp.jl:40
  Test threw exception
  Expression: withenv(var => tmp) do
        _tempdir()
    end == tmp
  tempdir path does not exist: C:\xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
  Stacktrace:
   [1] error(::String) at .\error.jl:33
   [2] _tempdir() at C:\Users\Mus\Desktop\testtmp.jl:22
   [3] #7 at .\none:0 [inlined]
   [4] withenv(::var"#7#8", ::Pair{String,String}) at .\env.jl:161
   [5] top-level scope at C:\Users\Mus\Desktop\testtmp.jl:40
   [6] include(::String) at .\client.jl:439
   [7] top-level scope at REPL[13]:1
   [8] eval(::Module, ::Any) at .\boot.jl:331
   [9] eval_user_input(::Any, ::REPL.REPLBackend) at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\REPL\src\REPL.jl:86
   [10] macro expansion at D:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.4\REPL\src\REPL.jl:118 [inlined]
   [11] (::REPL.var"#26#27"{REPL.REPLBackend})() at .\task.jl:358

ERROR: LoadError: There was an error during testing
in expression starting at C:\Users\Mus\Desktop\testtmp.jl:37

So I do get an error that the path is too long. The stat check works fine for me.

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 28, 2020

Member

Is it the intention that these paths be varying lengths? Is that the point of the test? Note that 1024 is not the max path length on most UNIX file systems, it's usually 255, so these tests are all kinds of wonky. Perhaps it would be easier to describe what the tests are trying to check for...

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 28, 2020 •
edited
Loading

Author Contributor

The tests are checking the buffer gets correctly resized in the tempdir loop, when the path returned from checking the ENV variable is longer than AVG_PATH (currently set to 260)

No they don't have to be varying in length, but I think I did this for additional test cases.

Yeah max path for linux is different, but I just used the same variable name as on Windows to unify all the tests.

We can just do the cases with i=0,8,9 and modify the linux MAX_PATH length if that's an issue you have encountered.

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 28, 2020

Member

My point was that 1024 is the incorrect max path length on Windows—it's 255 on most file systems.

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 28, 2020

Author Contributor

For this case we are using 260 as the max path length for windows. Take a look at the line
MAX_PATH = (Sys.iswindows() ? 260-9 : 1024) - length(PATH_PREFIX)

I spent a lot of time on this in the past, but this is the correct max path length to test on Windows. Please take a look at https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppathw

The maximum possible return value is MAX_PATH+1 (261).

We use 260, because of the terminating null

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 28, 2020

Author Contributor

I subtract 9 because we then add up to 9 additional chars in the test where we loop from i=0:8 and then i=9 (libuv bug, which has since been fixed.)

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 28, 2020

Member

Yes, I'm talking about UNIX: 1024 is not the max path length on UNIX.

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 29, 2020 •
edited
Loading

Author Contributor

Yea I'm aware, but as mentioned this is only testing buffer resizing on UNIX. Any value larger than AVGPATH would work here.

BTW I am confused because you asked about the windows situation (typo?) 2b08860#r37007822

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jan 29, 2020 •
edited
Loading

Member

I didn't mention Windows, but I can understand the confusion. The failure on Windows is actually just what I'd expect since the path doesn't exist. The confusing failure is non-Windows where the error is this:

IOError: stat: name too long (ENAMETOOLONG) for file "/tmp/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx123456789"
  Stacktrace:

That's why I'm asking if these tests were intentionally generating paths that are too long to possibly exist. It seems like the solution is to fix the tests on non-Windows systems not to generate temp path names that are too long and to possibly make it a warning if the path returned by tempdir doesn't exist instead of an error. But it's hard to see how that situation wouldn't almost immediately result in a worse and more confusing situation in short order, so it seems somewhat better to error early if tempdir() doesn't exist.

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 29, 2020 •
edited
Loading

Author Contributor

So in your PR change this line to the actual max path for the UNIX case.

 MAX_PATH = 260 - 9  - length(PATH_PREFIX)

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 29, 2020 •
edited
Loading

Author Contributor

In this PR, I originally did intend to test paths that are too large to exist because AVGPATH is currently set to 260 (we might want to reduce this number since we wouldn't expect most of the paths to be this long), which is the actual MAX_PATH so then in this case we never test the resize branch.

This comment has been minimized.

Sign in to view

Copy link
@musm

musm Jan 29, 2020

Author Contributor

We can maybe open a PR to discuss reducing AVG_PATH to a smaller more reasonable size (it represents the typical buffer size), which probably shouldn't be the theoretical max?
@test withenv(var => tmp) do
tempdir()
end == (tmp)
end
for i = 9
local tmp = PATH_PREFIX * "x"^MAX_PATH * "123456789"[1:i]
if Sys.iswindows()
# libuv bug
@test_broken withenv(var => tmp) do
tempdir()
end == tmp
else
@test withenv(var => tmp) do
tempdir()
end == tmp
end
end
end

let path = tempname()
# issue #9053
Expand Down

0 comments on commit 2b08860

Please sign in to comment.