Re: [Bug-tar] Bug in --remove-files with --append/-r

[Nathan Stratton Treadway]

On Fri, Sep 20, 2013 at 11:22:07 +0200, "Strand, Jörgen" wrote:
> When using -remove-files together with -r and -C wrong files will be removed.
> 
> > tar --version
> tar (GNU tar) 1.26
> Copyright (C) 2011 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
> 
> Written by John Gilmore and Jay Fenlason.
> > mkdir foo
> > echo bar > bar
> > echo foobar > foo/bar
> > tar -cf foo.tar --remove-files -C foo bar
> > find
> .
> ./bar
> ./foo
> ./foo.tar
> > echo foobar > foo/bar
> > tar -rf foo.tar --remove-files -C foo bar
> > find
> .
> ./foo
> ./foo/bar
> ./foo.tar

I just gave this a try with the latest git version of tar (commit
b6979c72), and see the same behavior there.  

(Specifically, after the second of these "tar" commands, far.tar file
does include a second copy of the ./foo/bar file, but the ./bar file is
the one deleted by the --remove-files processing.)



Internally, the problem seems to be that in --append mode,
update.c:update_archive() uses "name_from_list()" calls, which in turn
uses "chdir_do()" and thus the "virtual chdir" approach... but then the
program flow chains [via dump_file()..dump_file0()..queue_deferred_unlink()] 
to misc.c:normalize_filename() -- and that function hasn't been
converted to use the "virtual chdir" style yet (but simply uses
"xgetcwd()").

Thus, the entries queued for deferred unlinking are given a path
relative to the original working directory (which is still the
processes's current working directory) rather than the one specified by
the -C option.


In contrast, in --create mode [non-incremental], create_archive() uses
the names.c:name_next() function with the change_dirs argument set to
"1".  In this path, chdir_do() is never called, but
names.c:name_next_elt() makes actual "chdir()" calls for -C arguments...
and thus the entries later queued for deferred unlinking do have their
paths calculated relative to the directory specified by -C.


I looked around a little, and as far as I was able to find:
  * create.c:create_archive() and names_notfound()/label_notfound() within 
    names.c itself are the only places that call name_next() with 
    change_dirs set to 1
  * there aren't any hard-coded "name_next_elt (1)" calls
  * name_next_elt() contains the only remaining call to the
    plain "chdir()" function
  * misc.c:normalize_filename() contains the only remaining call to
    xgetcwd()/getcwd()

So I wonder if the "proper" fix at this point would be to switch
everything to using the "virtual chdir" approach?

(This seems a little strange, since it means changing the code path for
--create, which is the branch that is currently working correctly -- but
otherwise I think queue_deferred_unlink()/normalize_filename() would
need to include support for both "actual" and "virtual" chdirs styles,
which might be more painful in the long run.)

                                                        Nathan

----------------------------------------------------------------------------
Nathan Stratton Treadway  -  address@hidden  -  Mid-Atlantic region
Ray Ontko & Co.  -  Software consulting services  -   http://www.ontko.com/
 GPG Key: http://www.ontko.com/~nathanst/gpg_key.txt   ID: 1023D/ECFB6239
 Key fingerprint = 6AD8 485E 20B9 5C71 231C  0C32 15F3 ADCD ECFB 6239