Skip to content

mktmpdir has inconsistent behavior #15

Open

Description

@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}"
}

Activity

  1. syntaqx commented on Jun 30, 2019

    @syntaqx

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

  2. LukeShu commented on Jul 1, 2019

    @LukeShu
    Author

    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.

  3. syntaqx commented on Jul 31, 2019

    @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.

  4. LukeShu commented on Jul 31, 2019

    @LukeShu
    Author

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions