[Toybox] [PATCH] Fix mv on overwrite.

[Rob Landley]

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?

> 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

 1440934759.0