makeself-header.sh chdir actions are broken

[zenonian]

The following observations are based on the makeself 2.4.2 release.

In the makeself-header.sh script, the implementation of the MS_exec_cleanup function, and chdir handling in general, is broken.

In the open body of my Makefile, I define:

ARCHIVE_DIR     := foobar

Note the relative pathname.

Then later, in one of the Makefile rules, I execute:

./makeself.sh \
    --notemp \
    --sha256 \
    --help-header ,help_header \
    --cleanup ${CLEANUP_SCRIPT} \
    ${ARCHIVE_DIR} ${RELEASE_DIR}/${INSTALLER_FILE} "${INSTALLER_LABEL}" installer

That creates my wrapped program.

Later, when I run the wrapped program without arguments, I find that the following occurs, as observed in the scripting at the top:

TMPROOT=${TMPDIR:=/tmp}

Note that there is no requirement or validation that $TMPDIR is defined as an absolute path. That will be important later on.

The following line got substituted into by makeself.sh as it built the wrapped program:

targetdir="foobar"

Note the relative pathname, which came from this line in makeself.sh:

archdirname=`basename "$1"`

which means that it didn't really matter that ARCHIVE_DIR was a relative path in my Makefile, because in that line, makeself.sh cut it down to a relative path anyway.

Also note this line in the wrapped program; it will be relevant in the logic later on:

keep="y"

Back in my wrapped program, we hit this block of code to define $tmpdir:

if test x"$targetdir" = x.; then
    tmpdir="."
else
    if test x"$keep" = xy; then
        if test x"$nooverwrite" = xy && test -d "$targetdir"; then
            echo "Target directory $targetdir already exists, aborting." >&2
            exit 1
        fi
        if test x"$quiet" = xn; then
            echo "Creating directory $targetdir" >&2
        fi
        tmpdir="$targetdir"
        dashp="-p"
    else    
        tmpdir="$TMPROOT/selfgz$$$RANDOM"
        dashp=""
    fi
    mkdir $dashp "$tmpdir" || {
        echo 'Cannot create target directory' $tmpdir >&2
        echo 'You should try option --target dir' >&2
        eval $finish 
        exit 1
    }
fi

In the case under consideration, the tmpdir="$targetdir" branch was taken, so now we have tmpdir=foobar as our definition.

Toward the end of the scripting, we change our working directory to the temporary directory:

cd "$tmpdir"

And then at the end of the script, we do some cleanup:

MS_exec_cleanup

if test x"$keep" = xn; then
    cd "$TMPROOT"
    rm -rf "$tmpdir"
fi

Now let's look at the MS_exec_cleanup function:

MS_exec_cleanup() {
    if test x"$cleanup" = xy && test x"$cleanup_script" != x""; then
        cleanup=n
        cd "$tmpdir"
        eval "\"$cleanup_script\" $scriptargs $cleanupargs"
    fi
}

Yes, that's right, we're already in the foobar directory because of the cd foobar command executed previously in the main body of the script, which means that inside MS_exec_cleanup(), cd foobar as a relative pathname obviously fails there. And that causes the entire wrapped program to immediately die at that point, without doing whatever following cleanup was planned. That is the specific bug I am complaining about here.

It turns out that there is a location variable already defined with the path we need but not actually referenced anywhere in the script, that will serve our purposes here if we use it within the MS_exec_cleanup function

As long as we are in there looking at such code, contrast MS_exec_cleanup with MS_cleanup:

MS_cleanup()
{
    echo 'Signal caught, cleaning up' >&2
    MS_exec_cleanup
    cd "$TMPROOT"
    rm -rf "$tmpdir"
    eval $finish; exit 15
}

The MS_cleanup function is put into play by a trap, but only when we do not want to keep the tmpdir:

if test x"$keep" = xn; then
    trap MS_cleanup 1 2 3 15
fi

That logic is possibly wrong; what would happen in the earlier block of logic when we are defining tmpdir, had we executed

tmpdir="."

before even checking the value of $keep ? I haven't traced out that case myself, but I think it bears looking into.

But aside from that possibility, look at:

tmpdir="$TMPROOT/selfgz$$$RANDOM"

which means that if $TMPROOT is not an absolute pathname, then these lines in either MS_cleanup() or at the end of the scripting:

cd "$TMPROOT"
rm -rf "$tmpdir"

may well delete some unintended complete file tree (say, if $targetdir and thus $tmpdir is .), or perhaps nothing at all (since the rm command will be standing in the $TMPROOT directory, but if the $tmpdir path begins with $TMPROOT then its relative path will not be interpreted correctly relative to the current working directory).

Also, ignoring its effect on the value of $tmpdir, note what happens if $TMPROOT ended up itself being defined as a relative path. Since we may well be standing in the $tmpdir directory when cd $TMPROOT is executed, interpreting $TMPROOT itself as a relative path there can fail!

Finally, it is also worth looking at the implementation of $copy, wherein we define tmpdir as starting with $TMPROOT, and SCRIPT_COPY as starting with $tmpdir. Then we execute:

cd "$TMPROOT"
exec "$SCRIPT_COPY" --phase2 -- $initargs

That seems unlikely to work if TMPROOT does not contain an absolute path, because while standing in the $TMPROOT directory, the relative $SCRIPT_COPY path starting with $TMPROOT won't be found.

Naturally, I care most about fixing my own use case. But for general use, it looks to me like all possible code paths that involve chdir actions need to be reviewed.

With those things in mind, for purposes of fixing my own use cases, the following patch does the job.

--- makeself-header.sh.orig	2020-04-27 03:59:03.000000000 -0700
+++ makeself-header.sh	2020-11-16 07:08:20.746829455 -0800
@@ -8,6 +8,12 @@
     umask 077
 fi
 
+if test -n "\${TMPDIR}" -a "\`echo \"\${TMPDIR}\" | cut -c 1\`" != "/"; then
+    echo "ERROR:  The TMPDIR environment variable is defined as \"\${TMPDIR}\"."
+    echo "        But if TMPDIR is defined, it must be an absolute path."
+    exit 1
+fi
+
 CRCsum="$CRCsum"
 MD5="$MD5sum"
 SHA="$SHAsum"
@@ -286,6 +292,7 @@
 MS_exec_cleanup() {
     if test x"\$cleanup" = xy && test x"\$cleanup_script" != x""; then
         cleanup=n
+        cd "\$location"
         cd "\$tmpdir"
         eval "\"\$cleanup_script\" \$scriptargs \$cleanupargs"
     fi

[realtime-neil]

@zenonian , a few questions:

• Would you be able to implement your reproducer as a test unit?
• Would you be able to submit your patch as a PR?