Manually implement tempname on non-windows

[vchuravy]

Fixes #35785

[StefanKarpinski]

Shouldn't this do the opposite? If parent is passed, TMPDIR is ignored? If someone has explicitly asked for a temporary path to be in a certain directory, then we should respect that. They might do this, for example, if they need to ensure that it is on the same file system as a final location that they need to be able to move it to without copying. With this change, setting TMPDIR would wreck all attempts to do that.

[vchuravy]

Shouldn't this do the opposite? If parent is passed, TMPDIR is ignored? If someone has explicitly asked for a temporary path to be in a certain directory, then we should respect that. They might do this, for example, if they need to ensure that it is on the same file system as a final location that they need to be able to move it to without copying. With this change, setting TMPDIR would wreck all attempts to do that.

I am not sure I understand. This is doing exactly what you are describing.

If the user passes parent we will use that as the directory to prefix the name with, we ensure that libc honors the passed parent directory by unsetting TMPDIR since otherwise it would ignore parent and use TMPDIR instead. Since we use parent=tempdir() in the case that parent is not passed we still honor TMPDIR as an environment variable.

We could also achieve the same by setting TMPDIR to parent, and could then pass an empty string to libc.

[StefanKarpinski]

The comment says:

If TMPDIR is set tempname will ignore parent as an argument (#38873)

That's the opposite of what it should do: if parent is passed, ignore TMPDIR.

[vchuravy]

Currently:

vchuravy@odin ~> TMPDIR=~ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.5.3 (2020-11-09)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> tempname()
"/home/vchuravy/jl_Z9se0A"

julia> tempname("/opt")
"/home/vchuravy/jl_hI1x9x"

With this PR:

vchuravy@odin ~/s/julia (vc/tempname)> TMPDIR=~ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.5.3 (2020-11-09)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> @eval Base.Filesystem function tempname(parent::AbstractString=tempdir(); cleanup::Bool=true)
                  isdir(parent) || throw(ArgumentError("$(repr(parent)) is not a directory"))
                  # If TEMPDIR is set tempname will ignore `parent` as an argument (#38873)
                  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
tempname (generic function with 2 methods)

julia> tempname()
"/home/vchuravy/jl_u70Ddi"

julia> tempname("/opt")
"/opt/jl_WNbezj"

[vchuravy]

The comment says:

If TMPDIR is set tempname will ignore parent as an argument (#38873)

That's the opposite of what it should do: if parent is passed, ignore TMPDIR.

Ah! I see where the confusion comes from. I was documenting the behavior of :tempnam and not the behavior of the Julia function tempname.

[StefanKarpinski]

Ah, I understand the confusion: the comment is explaining why we need to unset TMPDIR whereas I read it as explaining how the function behaves. Perhaps there's some clearer way of wording that? Something like this:

Unless we unset TMPDIR here, it will cause tempnam to give a directory in that location, which would contradict the parent argument passed in here.

I worry about the thread safety of messing with TMPDIR like this. What if, for example, two different threads do this at the same time?

[vchuravy]

I worry about the thread safety of messing with TMPDIR like this. What if, for example, two different threads do this at the same time?

Yeah... setenv is not thread-safe at all.

[StefanKarpinski]

We could at least put a lock around this so that multiple calls to this function from different threads don't end up concurrently modifying the same environment variable.

[DilumAluthge]

Instead of setenv, would withenv work? Would that be thread-safe?

[vchuravy]

Instead of setenv, would withenv work? Would that be thread-safe?

I am using withenv here, but that uses setenv internally.

[DilumAluthge]

Instead of setenv, would withenv work? Would that be thread-safe?

I am using withenv here, but that uses setenv internally.

Ah fair enough

[staticfloat]

The issue is not just Julia code; if any C library (such as, for instance, tempname()) tries to read an environment variable while another C library (or some Julia task) modifies the environment block, we can segfault. We don't run into this very often because we tend to set environment variables at the beginning of program execution and leave them from then on, but the more we use withenv() the more likely we are to create this problem. To be explicit, a Julia mutex won't do anything but ensure that Julia can't interfere with other Julia operations. That's not even as helpful as you might think however, because since tempnam() uses getenv() to read the contents of $TMPDIR, if you have two Julia tasks that each wait for a mutex to call setenv() or getenv(), you can still have Julia acquire the (julia) mutex before calling setenv() and have that occur at a bad point in time for some other Julia task which is calling tempnam().

Possible solutions:

• We could surround all ccall()'s with acquiring a mutex and not release it until the ccall() returns. Ha ha, right. This at least avoids crashes being Julia's fault; C code could still start a thread that calls setenv() within it at some point and causes a segfault.

• We can hijack getenv() and setenv() somehow and force these functions to become threadsafe. A custom-build libc, symbol injection, etc... This is the only truly foolproof method that I know of, and will impose a performance penalty on environment actions, but IMO that's totally acceptable.

• We can avoid modification of environment variables at unsafe points (where "unsafe" means after any thread has been started by anything). Realistically, this means you should set environment variables in things like __init__() and leave it at that. I will note that I am guilty as anyone in that JLL wrappers set environment variables when launching external processes, but I've been working to eliminate that by passing an environment block to open(::Cmd) rather than using withenv, and in so doing avoiding setenv() on the parent process altogether.

[Keno]

Per discussion, should avoid using libc, since messing with the environment is inherently problematic.

[vchuravy]

Any good ideas on how to get a random number here? Musl use time_ns, but then mixes in two pointers as sources of randomness.

[musm]

Is there a particular reason we are avoiding Base.rand ?

[stevengj]

Even if we use Base.rand, we might want to initialize our own seed since we don't want the caller to be able to override this, no?

[vchuravy]

since we don't want the caller to be able to override this, no?

I don't think we care about the state of the rng. IIUC we just want a random seed that is not likely to be used by another process. This is why musl mixes in the two pointers to get some randomness in the higher bits, but we don't need to high quality randomness.

Is there a particular reason we are avoiding Base.rand ?

Base.rand is implemented through the stdlib Random and thus we would add a hard-dependency edge. Otherwise I would just use Random.randstring.

[vtjnash]

I believe Libc.rand() should provide suitable quality for this. It appears we can also use the same code everywhere now, rather than using separate, identical implementation for windows.

[musm]

We should use this implementation on Windows as well

[musm]

I find it a bit odd we are trying to implement tempnam since the first sentence of the description section in http://man7.org/linux/man-pages/man3/tempnam.3.html says "Never use this function." ... use mkstemp instead.

[vchuravy]

I find it a bit odd we are trying to implement tempnam since the first sentence of the description section in http://man7.org/linux/man-pages/man3/tempnam.3.html says "Never use this function." ... use mkstemp instead.

🤷 I read that as well and found it entertaining, I assume they want to discourage usage since there is the risk of running into collisions and mkstemp will lock the file immediatly.

[StefanKarpinski]

OMG, can we finish this?

[vchuravy]

OMG, can we finish this?

xD If anyone want to pick this up, please go ahead.

[staticfloat]

Rebased and unified a bit, as requested by reviewers.

[musm]

Any reason not to use this also on Windows?

[staticfloat]

Any reason not to use this also on Windows?

Despite the PR title, this does unify much of the windows/posix divide.

[staticfloat]

This is green, and I believe all reviewer concerns have been addressed. I'm going to add the merge me label.

state

[vchuravy]

Thanks Elliot for picking this up!

[DilumAluthge]

Thank you Valentin and Elliot!

[Keno]

CI failures might be related:
https://build.julialang.org/#/builders/44/builds/856
https://build.julialang.org/#/builders/72/builds/866
https://build.julialang.org/#/builders/34/builds/848

[Keno]

I will note that the file name is the same in both of these failures, so I'm assuming the RNG doesn't get seeded during precompile generation and since we're using tempname here, we probably end up clashing somewhere.

Edit: I may be too tired or I linked the wrong reports.

[Keno]

We may want to revert this until #43597 is merged. This is likely quite destabilizing to CI.