mktmpdir has inconsistent behavior

[LukeShu]

See: goreleaser/godownloader#104

TMPDIR is a shared temporary directory (specified by POSIX); being unset should be mostly equivalent to TMPDIR=/tmp. Many systems will set TMPDIR to a new temporary directory on each login; I have seen this on many GNU/Linux systems, and know that it is the default behavior on macOS. Multiple programs share TMPDIR for the duration of that login.

If a program uses more than 1 temporary file, it is likely desirable to contain them in a per-program-invocation temporary subdirectory of TMPDIR. In C, mkdtemp(3) (POSIX) is helpful for this purpose; in shell, the wrapper around that, mktemp -d (non-POSIX, but widely implemented) is helpful. The program should remember to remove that subdirectory before it exits.

mktmpdir has wildly different behavior when TMPDIR is set compared to when it's not set:

mktmpdir() {
  test -z "$TMPDIR" && TMPDIR="$(mktemp -d)"
  mkdir -p "${TMPDIR}"
  echo "${TMPDIR}"
}

• If TMPDIR is not already set: it is necessary to clean up the returned directory when the program is done; it has created a new temporary directory that would otherwise be leaked.
• If TMPDIR is already set: it is impermissible to clean up the returned directory when the program is done; it returns the shared temporary directory, other programs may be using it, the user may not have permission to remove it.

I believe the intent of the mktmpdir author was that TMPDIR be a private "static" variable to make mktmpdir return the same value if you call it multiple times (let us call this property "idempotence". However, that use conflicts with the use of TMPDIR as specified by POSIX. Maybe it was just sloppy thinking.

Besides idempotency, mktmpdir also has the property that it handles TMPDIR being set to a directory that does not exist; a scenario that mktemp -d does not handle.

A correct implementation would look like:

If idempotency is not a desired property:

mktmpdir() {
  test -z "$TMPDIR" && mkdir -p "$TMPDIR"
  mktemp -d
}

If idempotency is a desired property:

mktmpdir() {
  test -z "$TMPDIR" && mkdir -p "$TMPDIR"
  test -n "$_shlib_tmpdir" || _shlib_tmpdir="$(mktemp -d)"
  echo "${_shlib_tmpdir}"
}

[syntaqx]

Is this still an issue? Seems as though the PRs have been merged

[LukeShu]

A workaround was merged in to godownloader, but no PR was ever submitted upstream here. client9/shlib.git#branch=master still contains the original inconsistent behavior.

[syntaqx]

@LukeShu Can you link me to the fix in godownloader? Would love to backport a fix into shlib as well.

Nevermind - I see you tagged it, I"ll see what I can make from it.

[LukeShu]

The fix in godownloader (goreleaser/godownloader#105) was just "don't use mktmpdir".

In my opinion, both of the "correct implementations" I included in the issue body are "PR-worthy", but I'm not sure which behavior is desired.