[Toybox] [PATCH] Fix mv on overwrite.

[enh]

On Mon, Aug 31, 2015 at 12:20 PM, enh <enh at google.com> wrote:
> 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.)

i've sent a patch to make yesno printf-like because fixed-length
limits suck and existing uses of yesno often end up either formatting
into toybuf or writing to stderr themselves.

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



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