Re: new module

[Zoltan Hidvegi <hzoli@xxxxxxxxxx>]

Here are some pieces from Zefram's files.c:

>       + static char buf[PATH_MAX * 2];
>       + static char rbuf[PATH_MAX];
[...]
>       + dorm(char *nam, char *arg, char *ops)
[...]
>       + 		if(arg != buf)
>       + 		    strcpy(buf, arg);

You see I do not like this kind of code.  It works but it makes the code
more difficult to maintain.  buf is a static variable and sometimes arg and
buf is the same.  It is very easy to forget about that and it can cause you
headaches to track down a problem caused by this.  And it is almost
impossible to debug this if you call an other function which also happens
to use buf.  In files.c I do not see what's against using automatic
variable buffers.

There is one more argument against static buffers besides the clarity of
the code.  Zsh code must be as re-entrant as possible as a signal handler
can be called at any time.  E.g. while you busily doing an rm -rf foo a
signal may come and it may call an other rm (that probably violates POSIX
btw.).

I know that static buffers are used in some other places.  The result of
unmeta is in a static buffer if the string contained a metafied character.
But in other parts of the code this static buffer used immediately after
the unmeta call.

>       + 		*pos++ = '/';
>       + 		while((fn = zreaddir(d, 1))) {
>       + 		    if(ztrlen(fn) > space) {
>       + 			pos[-1] = 0;
>       + 			zwarnnam(nam, "%s: %e", buf, ENAMETOOLONG);
>       + 			err = 1;
>       + 			continue;
>       + 		    }
>       + 		    strcpy(pos, fn);

There are two problems with this code.  First it cannot remove a directory
hierarchy if it has too deep subdirectories.  That's quite bad when you use
it to clean up directories.  A user who wants to save his files from
automatic deletion can hide it deeply in a subdirectory.

The other problem if that when such an rm runs from root's cron ro clean up
/tmp it can be exploited to remove anything on the filesystem.  Any
component of a long filename can be replaced with a symlink while rm is
working which can be used to delete any tree on the system.

The right way to do that is to change into the directory we want to remove,
and stat . to make sure that the chdir did no go over a symlink.

Note that it is a more general problem: find ... -exec rm {} \; scripts are
always dangerous and can be exploited using some symlink trickery.
Therefore an option to rm which disables symlinks would be very useful
(which would allow rm -l symlink but not rm -l symlink/file).  If
implemented correctly, rm -lf <some_complicated_zsh_glob_pattern> would be
a safe operation.  Sine rm is a builtin long argument list is no longer a
problem.

E.g. when you type rm foo/bar the right procedure is:

lstat(foo), if a real directory, chpwd(foo), stat(.) and compare with the
previous lstat(foo), if not the same fail.  And now you can delete bar.

And to rm /foo/bar you have to chpwd(/) first.
Of couse a shell-builtin implementation should save pwd first and go back
after the operation is completed.

Btw. I've just noticed that rm -r in files.c uses stat() instead of lstat()
hence it follows symlinks happily :-(.

Zoltan