[Toybox] [PATCH] Fix mv on overwrite.

[enh]

On Sun, Aug 30, 2015 at 9:15 PM, Rob Landley <rob at landley.net> wrote:
> 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. :)

good point. an alternative would be to make yesno take a format
string. i can send you a patch to do that if you'd like to go that
route. (it would mean that the obvious code would then be correct even
in long-path cases.)

> 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



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.