patchoptimize gitdiff-do script

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>

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

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

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>

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

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 wrote:

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

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.

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?

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

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

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.

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.

Pasky wrote:

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

Good idea.

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

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.

Weeks ago, Pasky replied to pj:

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

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>:

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

MfG, JBG

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.

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

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

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

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.

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

Dave wrote:

Ok - useful point.

How about:

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

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?

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.

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

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:

Dave wrote:

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?

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:

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

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?

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

Care to provide one?

On Sat, Apr 16, 2005 at 08:33:25PM -0400, Dave Jones wrote:

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

Erik wrote:

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.

Paul Jackson <pj@sgi.com> wrote:

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 :)

Herbert wrote:

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

* Herbert Xu:

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