From 6fcbc209846b3bcc5cdf4deebfac940af5a6c4fe Mon Sep 17 00:00:00 2001 From: Valentin Churavy Date: Mon, 14 Dec 2020 16:12:14 -0500 Subject: [PATCH 1/5] Unset TMPDIR for tempname --- base/file.jl | 14 ++++++++++---- test/file.jl | 18 +++++++++++++----- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/base/file.jl b/base/file.jl index 74cf10f6ca8fa..894847da4e699 100644 --- a/base/file.jl +++ b/base/file.jl @@ -606,10 +606,16 @@ else # !windows # Obtain a temporary filename. function tempname(parent::AbstractString=tempdir(); cleanup::Bool=true) isdir(parent) || throw(ArgumentError("$(repr(parent)) is not a directory")) - p = ccall(:tempnam, Cstring, (Cstring, Cstring), parent, temp_prefix) - systemerror(:tempnam, p == C_NULL) - s = unsafe_string(p) - Libc.free(p) + # If TMPDIR is set libc's `tempnam` will ignore `parent` as an argument (#38873) + # So we unset the environment variable for the call to libc, after checking + # in `tempdir` for the default argument. + s = withenv("TMPDIR" => nothing) do + p = ccall(:tempnam, Cstring, (Cstring, Cstring), parent, temp_prefix) + systemerror(:tempnam, p == C_NULL) + s = unsafe_string(p) + Libc.free(p) + return s + end cleanup && temp_cleanup_later(s) return s end diff --git a/test/file.jl b/test/file.jl index 15dc5ef65bd5a..b4983ac5e61c5 100644 --- a/test/file.jl +++ b/test/file.jl @@ -106,15 +106,23 @@ using Random end @testset "tempname with parent" begin - withenv("TMPDIR" => nothing) do - t = tempname() - @test dirname(t) == tempdir() - mktempdir() do d + t = tempname() + @test dirname(t) == tempdir() + mktempdir() do d + t = tempname(d) + @test dirname(t) == d + end + @test_throws ArgumentError tempname(randstring()) + + # 38873: check that `TMPDIR` being set does not + # override the parent argument to `tempname`. + mktempdir() do d + whithenv("TMPDIR"=>tmpdir()) do t = tempname(d) @test dirname(t) == d end - @test_throws ArgumentError tempname(randstring()) end + @test_throws ArgumentError tempname(randstring()) end child_eval(code::String) = eval(Meta.parse(readchomp(`$(Base.julia_cmd()) -E $code`))) From cc97294956356d2a8f658d1cbed4ba09378c19af Mon Sep 17 00:00:00 2001 From: Valentin Churavy Date: Thu, 7 Jan 2021 23:10:51 -0500 Subject: [PATCH 2/5] implement tempname ourselve --- base/file.jl | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/base/file.jl b/base/file.jl index 894847da4e699..e557ffe9d1106 100644 --- a/base/file.jl +++ b/base/file.jl @@ -603,21 +603,40 @@ end else # !windows +# Based of musl __randname +function _rand_string() + r = Base.rand(UInt) # FIXME: https://git.musl-libc.org/cgit/musl/tree/src/temp/__randname.c + buf = Vector{Char}(undef, 6) + for i in 1:6 + buf[i] = 'A'+(r&15)+(r&16)*2; + r >>= 5 + end + String(buf) +end + # Obtain a temporary filename. function tempname(parent::AbstractString=tempdir(); cleanup::Bool=true) isdir(parent) || throw(ArgumentError("$(repr(parent)) is not a directory")) - # If TMPDIR is set libc's `tempnam` will ignore `parent` as an argument (#38873) - # So we unset the environment variable for the call to libc, after checking - # in `tempdir` for the default argument. - s = withenv("TMPDIR" => nothing) do - p = ccall(:tempnam, Cstring, (Cstring, Cstring), parent, temp_prefix) - systemerror(:tempnam, p == C_NULL) - s = unsafe_string(p) - Libc.free(p) - return s + + prefix = joinpath(parent, temp_prefix) + MAX_TRIES = 100 + + filename = nothing + for i in 1:MAX_TRIES + filename = string(prefix, _rand_string()) + if ispath(filename) + filename = nothing + else + break + end + end + + if filename === nothing + error("tempname: MAX_TRIES exhausted") end - cleanup && temp_cleanup_later(s) - return s + + cleanup && temp_cleanup_later(filename) + return filename end # Create and return the name of a temporary file along with an IOStream From 86e2ef4e2f92092642e0123defb800e65c705431 Mon Sep 17 00:00:00 2001 From: Elliot Saba Date: Thu, 16 Dec 2021 13:39:40 -0800 Subject: [PATCH 3/5] Unify windows/non-windows `tempname()` implementations --- base/file.jl | 95 ++++++++++++++++++---------------------------------- test/file.jl | 2 +- 2 files changed, 34 insertions(+), 63 deletions(-) diff --git a/base/file.jl b/base/file.jl index e557ffe9d1106..c3d246f61835b 100644 --- a/base/file.jl +++ b/base/file.jl @@ -554,76 +554,25 @@ end const temp_prefix = "jl_" -if Sys.iswindows() - -function _win_tempname(temppath::AbstractString, uunique::UInt32) - tempp = cwstring(temppath) - temppfx = cwstring(temp_prefix) - tname = Vector{UInt16}(undef, 32767) - uunique = ccall(:GetTempFileNameW, stdcall, UInt32, - (Ptr{UInt16}, Ptr{UInt16}, UInt32, Ptr{UInt16}), - tempp, temppfx, uunique, tname) - windowserror("GetTempFileName", uunique == 0) - lentname = something(findfirst(iszero, tname)) - @assert lentname > 0 - resize!(tname, lentname - 1) - return transcode(String, tname) -end - -function mktemp(parent::AbstractString=tempdir(); cleanup::Bool=true) - filename = _win_tempname(parent, UInt32(0)) - cleanup && temp_cleanup_later(filename) - return (filename, Base.open(filename, "r+")) -end - -# generate a random string from random bytes -function _rand_string() - nchars = 10 - A = Vector{UInt8}(undef, nchars) - windowserror("SystemFunction036 (RtlGenRandom)", 0 == ccall( - (:SystemFunction036, :Advapi32), stdcall, UInt8, (Ptr{Cvoid}, UInt32), - A, sizeof(A))) - - slug = Base.StringVector(10) +# Use `Libc.rand()` to generate random strings +function _rand_filename(len = 10) + slug = Base.StringVector(len) chars = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" - for i = 1:nchars - slug[i] = chars[(A[i] % length(chars)) + 1] + for i = 1:len + slug[i] = chars[(Libc.rand() % length(chars)) + 1] end - return name = String(slug) + return String(slug) end -function tempname(parent::AbstractString=tempdir(); cleanup::Bool=true) - isdir(parent) || throw(ArgumentError("$(repr(parent)) is not a directory")) - name = _rand_string() - filename = joinpath(parent, temp_prefix * name) - @assert !ispath(filename) - cleanup && temp_cleanup_later(filename) - return filename -end - -else # !windows - -# Based of musl __randname -function _rand_string() - r = Base.rand(UInt) # FIXME: https://git.musl-libc.org/cgit/musl/tree/src/temp/__randname.c - buf = Vector{Char}(undef, 6) - for i in 1:6 - buf[i] = 'A'+(r&15)+(r&16)*2; - r >>= 5 - end - String(buf) -end # Obtain a temporary filename. -function tempname(parent::AbstractString=tempdir(); cleanup::Bool=true) +function tempname(parent::AbstractString=tempdir(); max_tries::Int = 100, cleanup::Bool=true) isdir(parent) || throw(ArgumentError("$(repr(parent)) is not a directory")) prefix = joinpath(parent, temp_prefix) - MAX_TRIES = 100 - filename = nothing - for i in 1:MAX_TRIES - filename = string(prefix, _rand_string()) + for i in 1:max_tries + filename = string(prefix, _rand_filename()) if ispath(filename) filename = nothing else @@ -632,13 +581,36 @@ function tempname(parent::AbstractString=tempdir(); cleanup::Bool=true) end if filename === nothing - error("tempname: MAX_TRIES exhausted") + error("tempname: max_tries exhausted") end cleanup && temp_cleanup_later(filename) return filename end +if Sys.iswindows() +function _win_tempname(temppath::AbstractString, uunique::UInt32) + tempp = cwstring(temppath) + temppfx = cwstring(temp_prefix) + tname = Vector{UInt16}(undef, 32767) + uunique = ccall(:GetTempFileNameW, stdcall, UInt32, + (Ptr{UInt16}, Ptr{UInt16}, UInt32, Ptr{UInt16}), + tempp, temppfx, uunique, tname) + windowserror("GetTempFileName", uunique == 0) + lentname = something(findfirst(iszero, tname)) + @assert lentname > 0 + resize!(tname, lentname - 1) + return transcode(String, tname) +end + +function mktemp(parent::AbstractString=tempdir(); cleanup::Bool=true) + filename = _win_tempname(parent, UInt32(0)) + cleanup && temp_cleanup_later(filename) + return (filename, Base.open(filename, "r+")) +end + +else # !windows + # Create and return the name of a temporary file along with an IOStream function mktemp(parent::AbstractString=tempdir(); cleanup::Bool=true) b = joinpath(parent, temp_prefix * "XXXXXX") @@ -648,7 +620,6 @@ function mktemp(parent::AbstractString=tempdir(); cleanup::Bool=true) return (b, fdio(p, true)) end - end # os-test diff --git a/test/file.jl b/test/file.jl index b4983ac5e61c5..a7c0b6dca125d 100644 --- a/test/file.jl +++ b/test/file.jl @@ -117,7 +117,7 @@ end # 38873: check that `TMPDIR` being set does not # override the parent argument to `tempname`. mktempdir() do d - whithenv("TMPDIR"=>tmpdir()) do + withenv("TMPDIR"=>tempdir()) do t = tempname(d) @test dirname(t) == d end From 507028e0a86a71eae3b1ad8f5d059a45b756be35 Mon Sep 17 00:00:00 2001 From: Elliot Saba Date: Thu, 16 Dec 2021 16:59:35 -0800 Subject: [PATCH 4/5] Use array of bytes, rather than a string Co-authored-by: Steven G. Johnson --- base/file.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/file.jl b/base/file.jl index c3d246f61835b..18d0dad549546 100644 --- a/base/file.jl +++ b/base/file.jl @@ -557,7 +557,7 @@ const temp_prefix = "jl_" # Use `Libc.rand()` to generate random strings function _rand_filename(len = 10) slug = Base.StringVector(len) - chars = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" + chars = b"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" for i = 1:len slug[i] = chars[(Libc.rand() % length(chars)) + 1] end From a1b0da464b40000bf6c4beaf279ca4636fa27e76 Mon Sep 17 00:00:00 2001 From: Elliot Saba Date: Thu, 16 Dec 2021 17:12:52 -0800 Subject: [PATCH 5/5] Rename `_win_tempname()` -> `_win_mkstemp()` This was a confusingly-named function, since `tempname()` doesn't create a file (and is thus prone to race conditions) whereas `mkstemp()` creates a file, reserving that filename as used immediately. --- base/file.jl | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/base/file.jl b/base/file.jl index 18d0dad549546..16d264a119f0e 100644 --- a/base/file.jl +++ b/base/file.jl @@ -589,13 +589,17 @@ function tempname(parent::AbstractString=tempdir(); max_tries::Int = 100, cleanu end if Sys.iswindows() -function _win_tempname(temppath::AbstractString, uunique::UInt32) +# While this isn't a true analog of `mkstemp`, it _does_ create an +# empty file for us, ensuring that other simultaneous calls to +# `_win_mkstemp()` won't collide, so it's a better name for the +# function than `tempname()`. +function _win_mkstemp(temppath::AbstractString) tempp = cwstring(temppath) temppfx = cwstring(temp_prefix) tname = Vector{UInt16}(undef, 32767) uunique = ccall(:GetTempFileNameW, stdcall, UInt32, (Ptr{UInt16}, Ptr{UInt16}, UInt32, Ptr{UInt16}), - tempp, temppfx, uunique, tname) + tempp, temppfx, UInt32(0), tname) windowserror("GetTempFileName", uunique == 0) lentname = something(findfirst(iszero, tname)) @assert lentname > 0 @@ -604,7 +608,7 @@ function _win_tempname(temppath::AbstractString, uunique::UInt32) end function mktemp(parent::AbstractString=tempdir(); cleanup::Bool=true) - filename = _win_tempname(parent, UInt32(0)) + filename = _win_mkstemp(parent) cleanup && temp_cleanup_later(filename) return (filename, Base.open(filename, "r+")) end