[PATCH] fix mktemp (remove mktemp ;)

[PATCH] fix mktemp (remove mktemp ;)

[Paul Jackson]

Remove mktemp usage - it doesn't work on
some Mandrakes, nor on my SuSE 8.2 with
mktemp-1.5-531.

Replace with simple use of $$ (pid).
I've been using this same pattern for
20 years on many production scripts;
it's fast, solid and simple.

More robust tmp file removal, using trap,
so that scripts interrupted by signals
HUP, INT, QUIT or PIPE will cleanup.

Signed-off-by: Paul Jackson <pj@sgi.com>

Index: git-pasky-0.4/README
===================================================================
--- git-pasky-0.4.orig/README	2005-04-16 13:16:54.000000000 -0700
+++ git-pasky-0.4/README	2005-04-16 13:17:01.000000000 -0700
@@ -116,7 +116,6 @@ Software requirements:
 	C compiler
 	bash
 	basic shell environment (sed, gred, textutils, ...)
-	mktemp 1.5+ (Mandrake users beware!)
 	diff, patch
 
 
Index: git-pasky-0.4/gitcommit.sh
===================================================================
--- git-pasky-0.4.orig/gitcommit.sh	2005-04-12 10:39:14.000000000 -0700
+++ git-pasky-0.4/gitcommit.sh	2005-04-16 13:17:49.000000000 -0700
@@ -60,7 +60,9 @@ for file in $commitfiles; do
 	echo $file;
 done
 echo "Enter commit message, terminated by ctrl-D on a separate line:"
-LOGMSG=`mktemp -t gitci.XXXXXX`
+t=${TMPDIR:-/usr/tmp}/gitapply.$$
+trap 'rm -f $t.?; trap 0; exit 0' 0 1 2 3 15
+LOGMSG=$t.1
 if [ "$merged" ]; then
 	cat .git/merged | sed 's/^/Merging: /' >>$LOGMSG
 	cat .git/merged | sed 's/^/Merging: /'
@@ -86,7 +88,6 @@ if [ "$treeid" = "$(tree-id)" ] && [ ! "
 fi
 
 newhead=$(commit-tree $treeid $oldhead $merged <$LOGMSG)
-rm $LOGMSG
 rm -f .git/add-queue .git/rm-queue .git/merged
 
 if [ "$newhead" ]; then
Index: git-pasky-0.4/gitapply.sh
===================================================================
--- git-pasky-0.4.orig/gitapply.sh	2005-04-13 02:21:14.000000000 -0700
+++ git-pasky-0.4/gitapply.sh	2005-04-16 13:16:13.000000000 -0700
@@ -8,9 +8,11 @@
 #
 # Takes the diff on stdin.
 
-gonefile=$(mktemp -t gitapply.XXXXXX)
-todo=$(mktemp -t gitapply.XXXXXX)
-patchfifo=$(mktemp -t gitapply.XXXXXX)
+t=${TMPDIR:-/usr/tmp}/gitapply.$$
+trap 'rm -f $t.?; trap 0; exit 0' 0 1 2 3 15
+gonefile=$t.1
+todo=$t.2
+patchfifo=$t.3
 rm $patchfifo && mkfifo -m 600 $patchfifo
 
 show-files --deleted >$gonefile
@@ -74,4 +76,3 @@ while [ "$1" ]; do
 done
 '
 
-rm $pathfifo $todo $gonefile
Index: git-pasky-0.4/gitdiff-do
===================================================================
--- git-pasky-0.4.orig/gitdiff-do	2005-04-16 13:13:59.000000000 -0700
+++ git-pasky-0.4/gitdiff-do	2005-04-16 13:18:29.000000000 -0700
@@ -32,7 +32,9 @@ mkbanner () {
 	[ "$labelapp" ] && label="$label  ($labelapp)"
 }
 
-diffdir=$(mktemp -d -t gitdiff.XXXXXX)
+t=${TMPDIR:-/usr/tmp}/gitdiff.$$
+trap 'rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15
+diffdir=$t.1
 diffdir1="$diffdir/$id1"
 diffdir2="$diffdir/$id2"
 mkdir "$diffdir1" "$diffdir2"

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.650.933.1373, 1.925.600.0401

[PATCH] missing mkdir -p flag in gitdiff-do

[Paul Jackson]

First mkdir in gitdiff-do missing -p, so useless error

Signed-off-by: Paul Jackson <pj@sgi.com>

Index: git-pasky-0.4/gitdiff-do
===================================================================
--- git-pasky-0.4.orig/gitdiff-do	2005-04-16 13:18:29.000000000 -0700
+++ git-pasky-0.4/gitdiff-do	2005-04-16 13:19:07.000000000 -0700
@@ -37,7 +37,7 @@ trap 'rm -fr $t.?; trap 0; exit 0' 0 1 2
 diffdir=$t.1
 diffdir1="$diffdir/$id1"
 diffdir2="$diffdir/$id2"
-mkdir "$diffdir1" "$diffdir2"
+mkdir -p "$diffdir1" "$diffdir2"
 
 while [ "$1" ]; do
 	declare -a param

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.650.933.1373, 1.925.600.0401

[PATCH] optimize gitdiff-do script

[Paul Jackson]

Rewrite gitdiff-do so that it works with arbitrary
whitespace (space, tab, newline, ...) in filenames.

Reduce number of subcommands execv'd by a
third, by only calling 'rm' once, at end, not each
loop.

Avoid using shell arrays; perhaps more portable.

Avoid 'echo -e' when displaying names; dont expand escape
  sequences in names.

Use shell noglob (-f) to minimize getdents() calls.

Simplify argument parsing and tmp file management.

Comment the nastier shell patterns.

This reduces the time by about 1/3 of what it was.

Signed-off-by: Paul Jackson <pj@sgi.com>

Index: git-pasky-0.4/gitdiff-do
===================================================================
--- git-pasky-0.4.orig/gitdiff-do	2005-04-16 13:19:07.000000000 -0700
+++ git-pasky-0.4/gitdiff-do	2005-04-16 15:33:28.000000000 -0700
@@ -2,19 +2,22 @@
 #
 # Make a diff between two GIT trees.
 # Copyright (c) Petr Baudis, 2005
+# Copyright (c) Paul Jackson, 2005
 #
 # Takes two parameters identifying the two trees/commits to compare.
 # Empty string will be substitued to HEAD revision.
 #
 # Note that this is probably the most performance critical shell script
-# in the whole GIT suite. That's also why I resorted to bash builtin
-# features and stuff. -- pasky@ucw.cz
+# in the whole GIT suite.
 #
 # Outputs a diff converting the first tree to the second one.
 
+set -f   # keep shell from scanning "." to expand wildcards
 
-id1=$1; shift
-id2=$1; shift
+t=${TMPDIR:-/usr/tmp}/gitdiff.$$
+trap 'set +f; rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15
+
+id1=$1; id2=$2; shift 2
 
 # Leaves the result in $label.
 mkbanner () {
@@ -32,58 +35,55 @@ mkbanner () {
 	[ "$labelapp" ] && label="$label  ($labelapp)"
 }
 
-t=${TMPDIR:-/usr/tmp}/gitdiff.$$
-trap 'rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15
-diffdir=$t.1
-diffdir1="$diffdir/$id1"
-diffdir2="$diffdir/$id2"
-mkdir -p "$diffdir1" "$diffdir2"
-
-while [ "$1" ]; do
-	declare -a param
-	param=($1);
-	op=${param[0]:0:1}
-	mode=${param[0]:1}
-	type=${param[1]}
-	sha=${param[2]}
-	name=${param[3]}
-
-	echo -e "Index: $name\n==================================================================="
-
-	if [ "$type" = "tree" ]; then
-		# diff-tree will kindly diff the subdirs for us
-		# XXX: What about modes?
-		shift; continue
-	fi
-
-	loc1="$diffdir1/$name"; dir1="${loc1%/*}"
-	loc2="$diffdir2/$name"; dir2="${loc2%/*}"
-	([ -d "$dir1" ] && [ -d "$dir2" ]) || mkdir -p "$dir1" "$dir2"
-
-	case $op in
-	"+")
-		mkbanner "$loc2" $id2 "$name" $mode $sha
-		diff -L "/dev/null  (tree:$id1)" -L "$label" -u /dev/null "$loc2"
-		;;
-	"-")
-		mkbanner "$loc1" $id1 "$name" $mode $sha
-		diff -L "$label" -L "/dev/null  (tree:$id2)" -u "$loc1" /dev/null
-		;;
-	"*")
-		modes=(${mode/->/ });
-		mode1=${modes[0]}; mode2=${modes[1]}
-		shas=(${sha/->/ });
-		sha1=${shas[0]}; sha2=${shas[1]}
-		mkbanner "$loc1" $id1 "$name" $mode1 $sha1; label1=$label
-		mkbanner "$loc2" $id2 "$name" $mode2 $sha2; label2=$label
-		diff -L "$label1" -L "$label2" -u "$loc1" "$loc2"
-		;;
-	*)
-		echo "Unknown operator $op, ignoring delta: $1";;
-	esac
-
-	rm -f "$loc1" "$loc2"
-	shift
+for arg
+do
+  IFS='	'
+  set X$arg     	# X: don't let shell set see leading '+' in $arg
+  op="$1"
+  mode=${op#X?} 	# trim leading X? 1st two chars
+  type="$2"
+  sha="$3"
+  # if 4+ tabs, trim 1st 3 fields on 1st line with sed
+  case "$arg" in
+  *\	*\	*\	*\	*)
+    name=$(echo "$arg" |
+      /bin/sed '1s/[^	]*	[^	]*	[^	]*	//')
+    ;;
+  *)
+    name="$4"
+    ;;
+  esac
+
+  echo "Index: $name"
+  echo ===================================================================
+
+  test "$type" = "tree" && continue
+
+  loc1=$t.1
+  loc2=$t.2
+
+  case $op in
+  X+*)
+    mkbanner $loc2 $id2 "$name" $mode $sha
+    diff -L "/dev/null  (tree:$id1)" -L "$label" -u /dev/null $loc2
+    ;;
+  X-*)
+    mkbanner $loc1 $id1 "$name" $mode $sha
+    diff -L "$label" -L "/dev/null  (tree:$id2)" -u $loc1 /dev/null
+    ;;
+  X\**)
+    mode1=${mode%->*}	# trim '->' and after
+    mode2=${mode#*->}	# trim up to and including '->'
+    sha1=${sha%->*}	# trim '->' and after
+    sha2=${sha#*->}	# trim up to and including '->'
+
+    mkbanner $loc1 $id1 "$name" $mode1 $sha1; label1=$label
+    mkbanner $loc2 $id2 "$name" $mode2 $sha2; label2=$label
+    diff -L "$label1" -L "$label2" -u $loc1 $loc2
+    ;;
+  *)
+    badop=$(echo $op | sed 's/.\(.\).*/\1/')
+    echo "Unknown operator $badop, ignoring delta: $1"
+    ;;
+  esac
 done
-
-rm -rf "$diffdir"

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.650.933.1373, 1.925.600.0401

Re: optimize gitdiff-do script

[Petr Baudis]

Dear diary, on Sun, Apr 17, 2005 at 01:28:04AM CEST, I got a letter
where Paul Jackson <pj@sgi.com> told me that...
> Reduce number of subcommands execv'd by a
> third, by only calling 'rm' once, at end, not each
> loop.

The idea behind that was that diffing could take a significant portion
of disk space, especially when making large kernel diffs. Perhaps we
could make this a switch, but I would personally prefer defaulting to
the less space-consuming behaviour. I personally dislike applications
which like to pop 150M of nonsense to my /tmp.

Please don't reindent the scripts. It violates the current coding style
and the patch is unreviewable.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: optimize gitdiff-do script

[Paul Jackson]

Petr wrote:
> Please don't reindent the scripts. It violates the current coding style
> and the patch is unreviewable.

Sorry - I had not realized that there was a style in this case.

I am all in favor of such coding styles, and will gladly fit this one.

Do you want the patch resent, or a patch to restore indent on top of
this one?

> the patch is unreviewable.

The section that I indented the wrong way was such a total rewrite, that
you aren't going to be able to review it line by line compared to the
old anyway.  So in this case, it wasn't that I was modifying and
reindenting, rather that I was rewriting a page of code from scratch.

But that's a nit.  Honoring the coding style is necessary in any case.

> The idea behind that was that diffing could take a significant portion
> of disk space,

Here I don't understand, or don't agree, not sure which.

This won't eat more disk space, because the same tmp files are reused,
over and over.  Instead of unlinking them just before reopening them
truncating (O_WRONLY|O_CREAT|O_TRUNC), I just reopen them truncating.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: optimize gitdiff-do script

[Paul Jackson]

Pasky,

Looks like a couple of questions I asked over the weekend
got lost along the way.

 1) How do you want me to fix the indentation on my patch
    to optimize gitdiff-do script:
	- forget my first patch and resend from scratch, or
	- a second patch restoring indentation, on top of my first one.

 2) Would you be interested in a patch that used a more robust tmp
    file creation, along the lines of replacing

	    t=${TMPDIR:-/usr/tmp}/gitdiff.$$
	    trap 'set +f; rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15

    with:

	    tmp=${TMPDIR-/tmp}
	    tmp=$tmp/gitdiff-do.$RANDOM.$RANDOM.$RANDOM.$$
	    (umask 077 && mkdir $tmp) || {
		    echo "Could not create temporary directory! Exiting." 1>&2 
		    exit 1
	    }
	    trap 'rm -fr $tmp; trap 0; exit 0' 0 1 2 3 15
	    t=$tmp/tmp

    From the www.linuxsecurity.com link that Dave Jones provided, the
    above $tmp directory is about as good as using mktemp, while
    avoiding both dependency on mktemp options not everyone has.

 3) If interested in (2), would you want it instead of my previous mktemp
    removal patch, or on top of it?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: optimize gitdiff-do script

[Petr Baudis]

Dear diary, on Mon, Apr 18, 2005 at 05:23:34PM CEST, I got a letter
where Paul Jackson <pj@sgi.com> told me that...
> Pasky,
> 
> Looks like a couple of questions I asked over the weekend
> got lost along the way.

Yes, sorry about that; I had a lot of mail traffic lately and I'm not so
used to it. ;-)

>  1) How do you want me to fix the indentation on my patch
>     to optimize gitdiff-do script:
> 	- forget my first patch and resend from scratch, or
> 	- a second patch restoring indentation, on top of my first one.

Resend from scratch, please.

I cannot guarantee I will look at it immediately, though. Optimizing is
nice, but gitdiff-do's speed is already usable and there are much more
pressing issues for git-pasky right now.

>  2) Would you be interested in a patch that used a more robust tmp
>     file creation, along the lines of replacing
> 
> 	    t=${TMPDIR:-/usr/tmp}/gitdiff.$$
> 	    trap 'set +f; rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15
> 
>     with:
> 
> 	    tmp=${TMPDIR-/tmp}
> 	    tmp=$tmp/gitdiff-do.$RANDOM.$RANDOM.$RANDOM.$$
> 	    (umask 077 && mkdir $tmp) || {
> 		    echo "Could not create temporary directory! Exiting." 1>&2 
> 		    exit 1
> 	    }
> 	    trap 'rm -fr $tmp; trap 0; exit 0' 0 1 2 3 15
> 	    t=$tmp/tmp
> 
>     From the www.linuxsecurity.com link that Dave Jones provided, the
>     above $tmp directory is about as good as using mktemp, while
>     avoiding both dependency on mktemp options not everyone has.
> 
>  3) If interested in (2), would you want it instead of my previous mktemp
>     removal patch, or on top of it?

Instead of the previous patch. But what I said still holds - this can go
in only after we have a shell library sharing the common functions - I
don't want to have this horrid stuff in every file.

Actually, if you will make a mktemp shell function, no changes
whatsoever might be needed to the other scripts.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: optimize gitdiff-do script

[Paul Jackson]

Pasky wrote:
> But what I said still holds - this can go
> in only after we have a shell library sharing the common functions

Ah - thanks for repeating that - it didn't sink in the first time.

Good idea.

> Yes, sorry about that; I had a lot of mail traffic lately ...

No problem.  I hope you're having fun at the center of this cyclone.

> I cannot guarantee I will look at it immediately, though.

Good.  You priorities sound fine to me.

I'll rework the patches and send them along again in a few days,
when I get a chance.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: optimize gitdiff-do script

[Paul Jackson]

Weeks ago, Pasky replied to pj:
> >  1) How do you want me to fix the indentation on my patch
> >     to optimize gitdiff-do script:
> > 	- forget my first patch and resend from scratch, or
> > 	- a second patch restoring indentation, on top of my first one.
> 
> Resend from scratch, please.

As was already no doubt obvious to everyone but me,
I'm not going to get to this.  Sorry.  Good luck.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: [PATCH] fix mktemp (remove mktemp ;)

[Jan-Benedict Glaw]

On Sat, 2005-04-16 16:27:43 -0700, Paul Jackson <pj@sgi.com>
wrote in message <20050416232749.23430.93360.sendpatchset@sam.engr.sgi.com>:
> Index: git-pasky-0.4/gitcommit.sh
> ===================================================================
> --- git-pasky-0.4.orig/gitcommit.sh	2005-04-12 10:39:14.000000000 -0700
> +++ git-pasky-0.4/gitcommit.sh	2005-04-16 13:17:49.000000000 -0700
> @@ -60,7 +60,9 @@ for file in $commitfiles; do
>  	echo $file;
>  done
>  echo "Enter commit message, terminated by ctrl-D on a separate line:"
> -LOGMSG=`mktemp -t gitci.XXXXXX`
> +t=${TMPDIR:-/usr/tmp}/gitapply.$$

/usr/tmp/ ??? Hey, /usr may be mounted read-only!  Why not just use /tmp ?

MfG, JBG

-- 
Jan-Benedict Glaw       jbglaw@lug-owl.de    . +49-172-7608481             _ O _
"Eine Freie Meinung in  einem Freien Kopf    | Gegen Zensur | Gegen Krieg  _ _ O
 fuer einen Freien Staat voll Freier Bürger" | im Internet! |   im Irak!   O O O
ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));

Re: [PATCH] fix mktemp (remove mktemp ;)

[Paul Jackson]

> /usr/tmp/ ??? Hey, /usr may be mounted read-only!  Why not just use /tmp ?

Sure - that's fine to change.  Those that care will have TMPDIR set anyway.

I probably made that choice of /usr/tmp for the fallback 10 or 20 years
ago, and have never had reason to revisit it.  I have long forgotten why
I made that choice.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: fix mktemp (remove mktemp ;)

[Petr Baudis]

Dear diary, on Sun, Apr 17, 2005 at 01:27:43AM CEST, I got a letter
where Paul Jackson <pj@sgi.com> told me that...
> Remove mktemp usage - it doesn't work on
> some Mandrakes, nor on my SuSE 8.2 with
> mktemp-1.5-531.
> 
> Replace with simple use of $$ (pid).
> I've been using this same pattern for
> 20 years on many production scripts;
> it's fast, solid and simple.

And racy. And not guaranteed to come up with fresh new files.

> More robust tmp file removal, using trap,
> so that scripts interrupted by signals
> HUP, INT, QUIT or PIPE will cleanup.

But I like this!

I'm deferring those changes to the introduction of a git shell library,
which several people volunteered to do so far, but noone sent me any
patches for (the last one was probably Martin Mares, only few hours ago
though).

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: fix mktemp (remove mktemp ;)

[Paul Jackson]

> And racy. And not guaranteed to come up with fresh new files.

In theory perhaps.  In practice no.

Even mktemp(1) can collide, in theory, since there is no practical way
in shell scripts to hold open and locked the file from the instant of it
is determined to be a unique name.

The window of vulnerability for shell script tmp files is the lifetime
of the script - while the file sits there unlocked.  Anyone else with
permissions can mess with it.

More people will fail, and are already failing, using mktemp than I have
ever seen using $$ (I've never seen a documented case, and since such
files are not writable to other user accounts, such a collision would
typically not go hidden.)

Fast, simple portable solutions that work win over solutions with some
theoretical advantage that don't matter in practice, but also that are
less portable or less efficient.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: fix mktemp (remove mktemp ;)

[Dave Jones]

On Sat, Apr 16, 2005 at 05:02:21PM -0700, Paul Jackson wrote:
 > > And racy. And not guaranteed to come up with fresh new files.
 > 
 > In theory perhaps.  In practice no.
 > 
 > Even mktemp(1) can collide, in theory, since there is no practical way
 > in shell scripts to hold open and locked the file from the instant of it
 > is determined to be a unique name.

Using the pid as a 'random' number is a bad idea. all an attacker
has to do is create 65535 symlinks in /usr/tmp, and he can now
overwrite any file you own.

mktemp is being used here to provide randomness in the filename,
not just a uniqueness.

 > The window of vulnerability for shell script tmp files is the lifetime
 > of the script - while the file sits there unlocked.  Anyone else with
 > permissions can mess with it.

Attacker doesnt need to touch the script. Just take advantage of
flaws in it, and wait for someone to run it.

 > More people will fail, and are already failing, using mktemp than I have
 > ever seen using $$ (I've never seen a documented case, and since such
 > files are not writable to other user accounts, such a collision would
 > typically not go hidden.)
 > 
 > Fast, simple portable solutions that work win over solutions with some
 > theoretical advantage that don't matter in practice, but also that are
 > less portable or less efficient.

I'd suggest fixing your distributions mktemp over going with an
inferior solution.

		Dave

Re: fix mktemp (remove mktemp ;)

[Paul Jackson]

Dave wrote:
> mktemp is being used here to provide randomness in the filename,
> not just a uniqueness.

Ok - useful point.

How about:

	t=${TMPDIR:-/usr/tmp}/gitdiff.$$.$RANDOM

> all an attacker has to do is create 65535 symlinks in /usr/tmp

And how about if I removed the tmp files at the top:

	t=${TMPDIR:-/usr/tmp}/gitdiff.$$.$RANDOM
	trap 'rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15
	rm -fr $t.?

	... rest of script ...

How close does that come to providing the same level of safety, while
remaining portable over a wider range of systems, and not requiring that
a separate command be forked?

> I'd suggest fixing your distributions ...

It's not just my distro; it's the distros of all git users.

If apps can avoid depending on inessential details of their
environment, that's friendlier to all concerned.

And actually my distro is fine - it's just that I am running an old
version of it on one of my systems.  Newer versions of the mktemp -t
option.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: fix mktemp (remove mktemp ;)

[Dave Jones]

On Sat, Apr 16, 2005 at 05:44:09PM -0700, Paul Jackson wrote:
 > Dave wrote:
 > > mktemp is being used here to provide randomness in the filename,
 > > not just a uniqueness.
 > 
 > Ok - useful point.
 > 
 > How about:
 > 
 > 	t=${TMPDIR:-/usr/tmp}/gitdiff.$$.$RANDOM

pid is still predictable by watching ps output, $RANDOM is one of 32768
numbers, so it's still feasable to predict the result.
$RANDOM$RANDOM is better, and gets a little closer to mktemp strength randomness.

 > > all an attacker has to do is create 65535 symlinks in /usr/tmp
 > And how about if I removed the tmp files at the top:
 > 
 > 	t=${TMPDIR:-/usr/tmp}/gitdiff.$$.$RANDOM
 > 	trap 'rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15
 > 	rm -fr $t.?
 > 
 > 	... rest of script ...

Racy, though the chance of creating x thousand symlinks in such a small
window probably makes it a non-issue.

Actually.. http://www.linuxsecurity.com/content/view/115462/151/
has some interesting bits on temp dir creation without mktemp.
See section 3.4 onwards.

		Dave

Re: fix mktemp (remove mktemp ;)

[David Lang]

set your umask to make things only writeable by the same user.
then create a new directory (it will fail with an error if the directory 
already exists)

now you can create files in this directory without having to worry about 
other users makeing trouble for you (they can't create symlinks in this 
directory)

David Lang

On Sat, 16 Apr 2005, Dave Jones wrote:

> Date: Sat, 16 Apr 2005 20:57:57 -0400
> From: Dave Jones <davej@redhat.com>
> To: Paul Jackson <pj@sgi.com>
> Cc: pasky@ucw.cz, git@vger.kernel.org, mj@ucw.cz
> Subject: Re: fix mktemp (remove mktemp ;)
> 
> On Sat, Apr 16, 2005 at 05:44:09PM -0700, Paul Jackson wrote:
> > Dave wrote:
> > > mktemp is being used here to provide randomness in the filename,
> > > not just a uniqueness.
> >
> > Ok - useful point.
> >
> > How about:
> >
> > 	t=${TMPDIR:-/usr/tmp}/gitdiff.$$.$RANDOM
>
> pid is still predictable by watching ps output, $RANDOM is one of 32768
> numbers, so it's still feasable to predict the result.
> $RANDOM$RANDOM is better, and gets a little closer to mktemp strength randomness.
>
> > > all an attacker has to do is create 65535 symlinks in /usr/tmp
> > And how about if I removed the tmp files at the top:
> >
> > 	t=${TMPDIR:-/usr/tmp}/gitdiff.$$.$RANDOM
> > 	trap 'rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15
> > 	rm -fr $t.?
> >
> > 	... rest of script ...
>
> Racy, though the chance of creating x thousand symlinks in such a small
> window probably makes it a non-issue.
>
> Actually.. http://www.linuxsecurity.com/content/view/115462/151/
> has some interesting bits on temp dir creation without mktemp.
> See section 3.4 onwards.
>
> 		Dave
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies.
  -- C.A.R. Hoare

Re: fix mktemp (remove mktemp ;)

[Paul Jackson]

Dave wrote:
> http://www.linuxsecurity.com/content/view/115462/151/

Nice - thanks.

Pasky - would you be interested in a patch that used a more robust tmp
file creation, along the lines of replacing

	t=${TMPDIR:-/usr/tmp}/gitdiff.$$
	trap 'set +f; rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15

with:

	tmp=${TMPDIR-/tmp}
	tmp=$tmp/gitdiff-do.$RANDOM.$RANDOM.$RANDOM.$$
	(umask 077 && mkdir $tmp) || {
		echo "Could not create temporary directory! Exiting." 1>&2 
		exit 1
	}
	t=$tmp/tmp
	trap 'rm -fr $tmp; trap 0; exit 0' 0 1 2 3 15

If interested, would you want it instead of my previous mktemp removal
patch, or on top of it?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: fix mktemp (remove mktemp ;)

[Brian O'Mahoney]

No, you have to:
(a) create a unique, pid specific file name /var/tmp/myapp.$$.xyzzy
(b) create it in O_EXCL mode, so you wont smash another's held lock

(b-1) It worked, OK

(b-2) open failed, try ...xyzzz

repeat until (b-1)

There are thousands of examples of how to do this with bash.

Paul Jackson wrote:
> Dave wrote:
> 
>>mktemp is being used here to provide randomness in the filename,
>>not just a uniqueness.
> 
> 
> Ok - useful point.
> 
> How about:
> 
> 	t=${TMPDIR:-/usr/tmp}/gitdiff.$$.$RANDOM
> 
> 
>>all an attacker has to do is create 65535 symlinks in /usr/tmp

the point of the xyzzy seed is to make creating all possible files
in-feasable.

> 
> 
> And how about if I removed the tmp files at the top:
> 
> 	t=${TMPDIR:-/usr/tmp}/gitdiff.$$.$RANDOM
> 	trap 'rm -fr $t.?; trap 0; exit 0' 0 1 2 3 15
> 	rm -fr $t.?
> 
> 	... rest of script ...
> 
> How close does that come to providing the same level of safety, while
> remaining portable over a wider range of systems, and not requiring that
> a separate command be forked?
> 
> 
>>I'd suggest fixing your distributions ...
> 
> 
> It's not just my distro; it's the distros of all git users.
> 
> If apps can avoid depending on inessential details of their
> environment, that's friendlier to all concerned.
> 
> And actually my distro is fine - it's just that I am running an old
> version of it on one of my systems.  Newer versions of the mktemp -t
> option.
> 

-- 
mit freundlichen Grüßen, Brian.

Dr. Brian O'Mahoney
Mobile +41 (0)79 334 8035 Email: omb@bluewin.ch
Bleicherstrasse 25, CH-8953 Dietikon, Switzerland
PGP Key fingerprint = 33 41 A2 DE 35 7C CE 5D  F5 14 39 C9 6D 38 56 D5

Re: fix mktemp (remove mktemp ;)

[Paul Jackson]

> No, you have to:

How does this compare with the one I posted about 1 hour 30
minuts ago:

	tmp=${TMPDIR-/tmp}
	tmp=$tmp/gitdiff-do.$RANDOM.$RANDOM.$RANDOM.$$
	(umask 077 && mkdir $tmp) || {
		echo "Could not create temporary directory! Exiting." 1>&2 
		exit 1
	}
	t=$tmp/tmp
	trap 'rm -fr $tmp; trap 0; exit 0' 0 1 2 3 15

derived from the reference that Dave Jones provided?

> create it in O_EXCL mode,

What can one do that and hold that O_EXCL from within bash?

> There are thousands of examples of how to do this with bash.

Care to provide one?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: fix mktemp (remove mktemp ;)

[Erik van Konijnenburg]

On Sat, Apr 16, 2005 at 08:33:25PM -0400, Dave Jones wrote:
> On Sat, Apr 16, 2005 at 05:02:21PM -0700, Paul Jackson wrote:
>  > > And racy. And not guaranteed to come up with fresh new files.
>  > 
>  > In theory perhaps.  In practice no.
>  > 
>  > Even mktemp(1) can collide, in theory, since there is no practical way
>  > in shell scripts to hold open and locked the file from the instant of it
>  > is determined to be a unique name.
> 
> Using the pid as a 'random' number is a bad idea. all an attacker
> has to do is create 65535 symlinks in /usr/tmp, and he can now
> overwrite any file you own.
> 
> mktemp is being used here to provide randomness in the filename,
> not just a uniqueness.

How about putting using .git/tmp.$$ or similar as tempfile?

This should satisfy both the portability and security requirements,
since the warnings against using $$ only apply to public directories.

Regards,
Erik

Re: fix mktemp (remove mktemp ;)

[Paul Jackson]

Erik wrote:
> How about putting using .git/tmp.$$ or similar as tempfile?

One could, but best to normally honor the users TMPDIR setting.

Could one 'git diff' a readonly git repository?

Perhaps someone has a reason for putting their tmp files where
they choose - say a local file system in a heavy NFS environment.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: fix mktemp (remove mktemp ;)

[Herbert Xu]

Paul Jackson <pj@sgi.com> wrote:
> 
> Even mktemp(1) can collide, in theory, since there is no practical way
> in shell scripts to hold open and locked the file from the instant of it
> is determined to be a unique name.

mktemp(1) creates the file before exiting.  Other instances of mktemp(1)
cannot successfully create the same file (they all use O_EXCL).
Therefore this race does not exist, even in theory :)
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: fix mktemp (remove mktemp ;)

[Paul Jackson]

Herbert wrote:
> mktemp(1) creates the file before exiting.  ... O_EXCL

Aha - right you are.  Thanks for pointing that out.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: fix mktemp (remove mktemp ;)

[Florian Weimer]

* Herbert Xu:

> Paul Jackson <pj@sgi.com> wrote:
>> 
>> Even mktemp(1) can collide, in theory, since there is no practical way
>> in shell scripts to hold open and locked the file from the instant of it
>> is determined to be a unique name.
>
> mktemp(1) creates the file before exiting.  Other instances of mktemp(1)
> cannot successfully create the same file (they all use O_EXCL).
> Therefore this race does not exist, even in theory :)

/tmp cleaners exist, but the risks are minimal for programs which
aren't SUID/SGID.