[Toybox] [PATCH] Fix mv on overwrite.

[enh]

On Sun, Aug 30, 2015 at 4:39 AM, Rob Landley <rob at landley.net> wrote:
> On 08/28/2015 08:02 PM, enh wrote:
>> two patches here. the first patch is clearly a desirable bug fix but
>
> Er, yes. Sorry about that.
>
>> there are some trickier compatibility choices lurking in the same
>> area. there's a bigger patch below, but i've provided both in case the
>> latter requires too much thought/experimentation :-)
>
> I read through so many weird rm corner cases making that work. And yes,
> posix and gnu disagreed on some of them:
>
> http://landley.net/notes-2012.html#06-12-2012
>
> Oh, and the fix to the infinite directory traversal thing is to check
> the stat info in the parent dirtree node when you traverse .. to go back
> up and if it doesn't match traverse back down from the top as far as you
> can confirm the matches for.
>
> What you do about there being a significant gap between the two,
> however, I'm honestly not sure about. (I need test cases. If somebody
> just did a mv of your parent directory three levels up entirely out of
> the tree you're considering, the behavior should still be correct,
> essentially deferring the mv until after the traversal completes. If
> somebody does a mv elsewhere within the same tree... that's pilot error,
> and we avoid a rm or cp wandering outside the tree.)
>
> I lean towards "if you have a significant gap, abort the darn directory
> tree traversal because somebody did something stupid". Except rsync
> should ignore it, because that's a best-effort thing on potentially
> active filesystems. Maybe I need a DIRTREE_PERSEVERE flag? (And possibly
> DIRTREE_INFINITE flag to not cache the filehandles, although that could
> also be a dynamic "once you hit ~50 levels deep, switch modes" thing...
> except we don't currently have global dirtree traversal state...)
>
> My todo list has archaeological _layers_.
>
>> (facebook found this bug:
>> https://code.google.com/p/android-developer-preview/issues/detail?id=3096)
>
> I apologize to facebook.
>
> That report implies that Android M is frozen already. It looks like git
> 05499787ca89 is what you're shipping for M?

no, looks like this is the last upstream change merged into M:

commit 20019be7c8667b70ff68692b24029aed2c857639
Author: Rob Landley <rob at landley.net>
Date:   Thu May 14 13:48:55 2015 -0500

    Bugfix from Hyejin Kim: su should not prompt root user for new
user's password.

at some point every release cycle, AOSP master no longer gets merged
into the release branch. it's still possible to cherrypick, but that
gets increasingly difficult as time passes. those of us on the lower
levels try to stabilize early. (this is also why i'm keen to get stuff
like uptime and lsof switched over now --- not because they have any
hope of being in M, but to maximize the testing they can get in master
before N ever opens for business.)

folks relying on toybox for automated testing should be shipping their
own known binary rather than relying on whatever's on the system, and
one day i'd like to get that into the NDK so it's easy for random
developers too.

>> Fix mv on overwrite.
>>
>> We need to remove the destination, not the source, to be able to overwrite.
>>
>> 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*
>> diff --git a/toys/posix/cp.c b/toys/posix/cp.c
>> index d5e92f2..5a55f40 100644
>> --- a/toys/posix/cp.c
>> +++ b/toys/posix/cp.c
>> @@ -382,7 +382,7 @@ void cp_main(void)
>>          {
>>            fprintf(stderr, "%s: overwrite '%s'", toys.which->name, TT.destname);
>>            if (!yesno("", 1)) rc = 0;
>> -          else unlink(src);
>> +          else unlink(TT.destname);
>>          }
>>        }
>
> Applied.
>
> I need to look at the second one more closely after dinner.
>
> (I'm currently implementing the darn aboriginal linux toybox-test build
> control image so I can run tests as root in a known environment, and
> then I need to tackle the horrible state of the testsuite which is
> failing all sorts of stuff and needs fixing. Then I need to finish the
> nommu support because it's got my tree screwed up and not building
> easily until that works through. And THEN I need to finish the cp
> --preserve xargs support. And then I need to flush the other half-dozen
> pending commits out of my tree...)
>
> Thanks,
>
> Rob



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