Re: path_concat proposed cleanup for corutils

[Jim Meyering]

Paul Eggert <address@hidden> wrote:
> I wanted to use path_concat in Bison to fix a porting problem to DOS,
> and noticed that path_concat had the same problem: it mishandled the
> case where BASE starts with "c:".  While fixing this, I noticed a few
> other problems:
>
> * cp, install, and ls all assume that path_concat never returns NULL,
>   and they can dump core if path_concat fails due to memory exhaustion.
>
> * There is a function xpath_concat defined in path-concat.c but it's not
>   declared and nobody uses it.  I think it's simpler to have path_concat
>   always return a nonnull pointer.

I'd prefer to continue the x-prefixing-functions-that-may-exit tradition,
but since no application requires the non-exiting version of path_concat,
I suppose it can wait and be renamed later if/when the need arises.

> * cp.c has a potential integer overflow if the dir name has more than 2 GiB,
>   and will trash the resulting file name.

FYI, there are other problems with copy when file names become very long.
The most serious are that it's limited by PATH_MAX and that it uses
alloca for things that might get bigger than 4k (long file names).
There's already a note (I've just made it a FIXME) to convert copy.c
to fix both problems by switching to use fts.

> * Only nohup.c passes a NULL pointer as the first argument to
>   path_concat, but it is a glitch, and it causes nohup to try to open
>   the same file twice.  I rewrote nohup to avoid the glitch, so that
>   path_concat can now assume that its first argument is non NULL.
>
> * There is a (perhaps theoretical?) problem if some standard header
>   defines mempcpy as a macro, but there is no mempcpy function.
>
> * Sometimes more than one slash is inserted between the dir and the base,
>   if they started with multiple slashes there.  This is just a minor annoyance
>   but while we're in the neighborhood....
>
> Here is a patch.
>
> Index: ChangeLog
> ===================================================================
> RCS file: /home/meyering/coreutils/cu/ChangeLog,v
> retrieving revision 1.976
> diff -p -u -r1.976 ChangeLog
> --- ChangeLog 2 Jul 2004 17:01:30 -0000       1.976
> +++ ChangeLog 2 Jul 2004 21:02:30 -0000
> @@ -1,3 +1,16 @@
> +2004-07-02  Paul Eggert  <address@hidden>
> +
> +     * src/copy.c (copy_dir): Assume path_concat returns non-NULL.
> +     * src/cp.c (do_copy): Likewise.
> +     * src/mv.c (movefile): Likewise.
> +
> +     * src/cp.c (make_path_private): 2nd arg is now size_t, not int,
> +     to avoid problem when path_concat dir name is longer than 2 GiB (!).
> +
> +     * src/nohup.c (main): Don't pass NULL first argument to path_concat.
> +     This cleans up the semantics a bit, as we no longer try to open the
> +     same file twice.

Applied.
Thanks!

Only one nit.
I'm not sure what you intended by the `cd DIR; cat BASE' part
of the comment for the new path_concat.

   The resulting file name is equivalent to what you would get by
   running (cd DIR; cat BASE), where BASE is ABASE with any file system
   prefixes and leading separators removed.