Discussion:
install-sh and $RANDOM
(too old to reply)
c***@SDF.ORG
2016-10-17 20:58:00 UTC
Permalink
Raw Message
Hi,

in build-aux/install-sh scriptversion=2016-01-11.22 line 327
tmpdir=${TMPDIR-/tmp}/ins$RANDOM-$$
... things which are spuriously bad if $RANDOM does not exist ...

Please use something like:
tmpdir=$(mktemp -d -p ${TMPDIR:-/tmp})

Note also :

NetBSD /bin/sh does not have $RANDOM.
I don't know how portable mktemp is. sorry.

Thanks.
Eric Blake
2016-10-17 21:56:05 UTC
Permalink
Raw Message
Post by c***@SDF.ORG
Hi,
in build-aux/install-sh scriptversion=2016-01-11.22 line 327
tmpdir=${TMPDIR-/tmp}/ins$RANDOM-$$
... things which are spuriously bad if $RANDOM does not exist ...
Thanks for the report. However, you've made two mistakes.

First, gnulib does not maintain install-sh - that is automake's job, so
if anything needs to change, it would have to change upstream in
automake first.

Second, your claim that things are "spuriously bad if $RANDOM does not
exist" is false. Look at the full context:

tmpdir=${TMPDIR-/tmp}/ins$RANDOM-$$
trap 'ret=$?; rmdir "$tmpdir/d" "$tmpdir" 2>/dev/null; exit
$ret' 0

if (umask $mkdir_umask &&
exec $mkdirprog $mkdir_mode -p -- "$tmpdir/d")
Post by c***@SDF.ORG
/dev/null 2>&1
then
if test -z "$dir_arg" || {
...
fi
rmdir "$tmpdir/d" "$tmpdir"
else
# Remove any dirs left behind by ancient mkdir
implementations.
rmdir ./$mkdir_mode ./-p ./-- 2>/dev/null
fi
trap '' 0;;
esac;;
esac

if
$posix_mkdir && (
umask $mkdir_umask &&
$doit_exec $mkdirprog $mkdir_mode -p -- "$dstdir"
)
then :
else

# The umask is ridiculous, or mkdir does not conform to POSIX,
# or it failed possibly due to a race condition. Create the
# directory the slow way, step by step, checking for races as we go.

Whether the directory is named '/tmp/ins1234-5678' (on bash) or
'/tmp/ins-5678' (on shells that lack $RANDOM), the remaining code is
STILL atomically correct - either the mkdir succeeds or fails, but there
is NO way that the script can be coerced into mistakenly acting on an
unintended file because someone was able to predict the directory name.
True, collisions are more likely on a setup without $RANDOM, but the
result of a collision is merely that the script gracefully falls back to
slower code, NOT that it operates unsafely on the colliding name.

You're not the first person to complain that $RANDOM is a bashism, and
this is not the first time we've had to retort that our use of $RANDOM
is a nicety, but not a necessity, and that the code is perfectly safe
and tested on shells where the expansion of $RANDOM is the empty string.
Post by c***@SDF.ORG
tmpdir=$(mktemp -d -p ${TMPDIR:-/tmp})
NetBSD /bin/sh does not have $RANDOM.
I don't know how portable mktemp is. sorry.
Less portable than $RANDOM expanding to the empty string.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Ben Pfaff
2016-10-17 23:06:29 UTC
Permalink
Raw Message
Post by Eric Blake
You're not the first person to complain that $RANDOM is a bashism, and
this is not the first time we've had to retort that our use of $RANDOM
is a nicety, but not a necessity, and that the code is perfectly safe
and tested on shells where the expansion of $RANDOM is the empty string.
I can see why it would surprise people, so maybe it would be helpful to
add a comment mentioning that the code is still safe when $RANDOM is not
useful, and perhaps pointing to this discussion.
c***@SDF.ORG
2016-10-18 04:46:40 UTC
Permalink
Raw Message
Post by Eric Blake
Second, your claim that things are "spuriously bad if $RANDOM does not
tmpdir=${TMPDIR-/tmp}/ins$RANDOM-$$
trap 'ret=$?; rmdir "$tmpdir/d" "$tmpdir" 2>/dev/null; exit
$ret' 0
I don't mean that it's dangerous to use (endangers the user), but deleting
those directories when $tmpdir is just /tmp/ins- will make this script race
other instances of itself, and delete their work.

(I didn't find the script in automake's git repository, so assumed this
is the place)
c***@SDF.ORG
2016-10-18 11:25:11 UTC
Permalink
Raw Message
Post by c***@SDF.ORG
Post by Eric Blake
Second, your claim that things are "spuriously bad if $RANDOM does not
tmpdir=${TMPDIR-/tmp}/ins$RANDOM-$$
trap 'ret=$?; rmdir "$tmpdir/d" "$tmpdir" 2>/dev/null; exit
$ret' 0
I don't mean that it's dangerous to use (endangers the user), but deleting
those directories when $tmpdir is just /tmp/ins- will make this script race
other instances of itself, and delete their work.
Except that it won't be just /tmp/ins-, but /tmp/ins-$$ (that is, the
pid is encoded into each directory); parallel runs of this script have
different pids and thus different directories.
Post by c***@SDF.ORG
(I didn't find the script in automake's git repository, so assumed this
is the place)
automake.git/lib/install-sh
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Ah!

Sorry for the noise then.

Loading...