[Toybox] [PATCH] Fix mv on overwrite.

[Rob Landley]

On 08/28/2015 08:02 PM, enh wrote:
> Fix mv on overwrite and its prompt.
> 
> We need to remove the destination, not the source, to be able to overwrite.
> 
> Also, coreutils mv doesn't prompt if it's not talking to a tty. This
> change also affects killall, crontab, find, and rm. The rm case at
> least is interesting --- coreutils silently *does* do the removal,
> whereas this patch would make toybox silently *not* do the removal.
> This despite the fact that coreutils rm on a tty defaults to 'n'. So
> do we want two different defaults for yesno? Or is this a coreutils
> bug? (It certainly seems surprising to me.)
> 
> diff --git a/lib/lib.c b/lib/lib.c
> index c16cffe..85ea4e1 100644
> --- a/lib/lib.c
> +++ b/lib/lib.c
> @@ -634,6 +634,8 @@ int yesno(char *prompt, int def)
>  {
>    char buf;
> 
> +  if (!isatty(0)) return def;
> +
>    fprintf(stderr, "%s (%c/%c):", prompt, def ? 'Y' : 'y', def ? 'n' : 'N');
>    fflush(stderr);
>    while (fread(&buf, 1, 1, stdin)) {

Reasonable.

> diff --git a/tests/mv.test b/tests/mv.test
> index 53fc999..030e9cc 100755
> --- a/tests/mv.test
> +++ b/tests/mv.test
> @@ -96,3 +96,10 @@ touch file1 file2
>  testing "Move -n file new_file (exist)" "mv -n file1 file2 &&
>     [ -e file1 -a -e file2 ] && echo 'yes'" "yes\n" "" ""
>  rm -f file*
> +
> +touch file1 file2
> +chmod 400 file1 file2
> +testing "Move file over unwritable file with no stdin" \
> +   "</dev/null mv file2 file1 && [ -e file -a ! -e file2 ] && echo 'yes'" \
> +   "yes\n" "" ""
> +rm -f file*

Already checked in as part of earlier commit.

> diff --git a/toys/posix/cp.c b/toys/posix/cp.c
> index d5e92f2..c9de14d 100644
> --- a/toys/posix/cp.c
> +++ b/toys/posix/cp.c
> @@ -380,9 +380,10 @@ void cp_main(void)
>          if (!stat(TT.destname, &st)
>            && ((toys.optflags & FLAG_i) || !(st.st_mode & 0222)))
>          {
> -          fprintf(stderr, "%s: overwrite '%s'", toys.which->name, TT.destname);
> -          if (!yesno("", 1)) rc = 0;
> -          else unlink(src);
> +          snprintf(toybuf, sizeof(toybuf), "%s: overwrite '%s'",
> +                   toys.which->name, TT.destname);
> +          if (!yesno(toybuf, 1)) rc = 0;
> +          else unlink(TT.destname);
>          }
>        }

The reason I didn't do that (and the reason yesno() was initially
designed to let you print the start of the message yourself) is the name
isn't guaranteed to fit into toybuf. You're trimming to the
sizeof(toybuf) in the snprintf (I just did five minutes of digging
because the snprintf() man page is very unclear about whether the null
terminator is guaranteed to be written, but luckily the posix page on it
_is_ clear and it is in the nonzero size case).

That said, truncating the filename is breakage. (Long path only shows
path, no idea what file we're referring to.) I'm intentionally doing an
openat() based approach to support arbitrarily deep filenames because
you can create them after the fact with directory mv so you simply can't
avoid 'em, and we just gotta cope.

That said have an xmprintf() in lib/xwrap.c for exactly this purpose.
It's sad to churn malloc/free but the -i case is very much _not_ a
hotpath, so... (Or I could yank -i if !isatty(0) in main. :)

But before I check any of that in... I have half a human_readable()
change in my lib/lib.c. Right, need to clean up all this half-finished
stuff in my workspace...

Rob

 1440994507.0