Untar pending changes - Was: Cutting 4.11.0 Soon

[Chris Johns]

On 20/11/2015 2:56 AM, Pavel Pisa wrote:
> Hello Chris,
> 
> On Wednesday 18 of November 2015 23:15:12 Chris Johns wrote:
>> On 19/11/2015 8:39 AM, Pavel Pisa wrote:
>>> The patch is important for unpacking standard tar command generated
>>> archives used for example by some of Microwindows tests.
>>
>> I do not think the patch is enough. For example on OS X:
>>
>> $ rm -rf x && mkdir x && mkdir x/1 && touch x/1/1 touch x/2 && tar cf
>> t1.tar x
>> $ rm -rf x && mkdir x && mkdir x/2 && touch x/2/1 touch x/1 && tar cf
>> t2.tar x
>> $ rm -rf x && tar xf t1.tar && tar xf t2.tar
>> x/1: Can't remove already-existing dir
>> tar: Error exit delayed from previous errors.
>>
>> and on FreeBSD:
>>
>> $ rm -rf x && mkdir x && mkdir x/1 && touch x/1/1 touch x/2 && tar cf
>> t1.tar x
>> $ rm -rf x && mkdir x && mkdir x/2 && touch x/2/1 touch x/1 && tar cf
>> t2.tar x
>> $ rm -rf x && tar xf t1.tar && tar xf t2.tar
>> x/1: Can't replace existing directory with non-directory
>> tar: Error exit delayed from previous errors.
>>
>> I think we need to add more checking and error if nodes are not the
>> same. I cannot see a way around this.
>>
>> The current implementation is overly cautious and it has made me move
>> away a direction and untar a fresh image. In the end I think it is
          ^ directory
>> better as stray files do not hang around. I would rather we handle the
>> special cases correctly or not at all.
> 
> I am not sure if I understand to your reply right.

I meant directory and not direction. Sorry about that.

I think we agree and it is just the detail and how much we want to do
that needs to be resolved.

> Actual situation is that Untar_FromFile() ignores mkdir() error.

Yes and if a file is replaced with a directory things break.

> Untar_FromMemory() and rtems_tarfs_load() fails if there are directory
> or file in a way.

Yes the conservative approach.

> My proposal is to allow previous existence of directory at a path
> which is specified as a directory in a TAR file. At least this is intention
> of the patch

It seems to be this.

> 
> https://devel.rtems.org/attachment/ticket/2413/0001-untar-do-not-exit-with-error-when-created-directory-.patch
> 
> If patch is applied then preceding existence of other than directory
> object on the path specified by TAR archive would lead to consistent untar
> fail in all cases. On the other hand, it is allowed that directory already
> exists in place where they would be created by untar.
> This behavior should be compatible with POSIX notation.

I agree.

> There is question, if ownership and mode should be updated in such
> case but RTEMS did not care about these information stored in
> TAR till now.

We need to deal with uid and gid. I have gear in the field where this is
important.

> 
> rtems_tarfs_load() seems take little care about operation
> result for case of regular file (linkflag == REGTYPE). It tries
> to free location and then map content without copying. (Nice feature
> for liked in TAR.) But problem to free or create new
> node is ignored silently. I am not sure if rtems_filesystem_location_free()
> could remove directory subree if it is in a way of regular file.

I do not know.

> Problem to create symlink is reported.

Ok.

> rtems_tarfs_load() can target only IMFS and from data stored in memory
> which stay constant for whole life of FS.

Yes.

> Other untar implementations are generic utilities without this
> limitation but the copy allocate memory and are much more memory hungry
> when compared with above IMFS special case.

I use the untar to populate a JFF2 disk.

> These other implementations use creat() and fopen(.., "w")
> for regular files. Untar_FromFile() is silent in the case of file creation
> problem. Untar_FromMemory() stops and reports error in case of problem
> to create file. Symlinks creation problems are silently ignored.

Ok.

> The problem that tar containing "/" or "./" directory entries
> leads to permanent fail is what I stubled over in our projects.

Yes and we need to fix this. It is a valid use case.

> Please, describe what do you prefer and if something should
> be done for 4.11 or only master should be considered.

I attach a script showing the issues I am attempting to describe. If you
look at test 1 and 2 a file can be replaced by a directory and an empty
directory can be replaced by a file but a directory that is not empty
cannot be replaced by a file. There are a few other combinations and I
am sure there are others I have not covered.

I am wondering if the logic in the patch that does mkdir, checks the
result then does a stat if required should be a stat and then some logic
based on the results of the tests I have provided?

Chris
-------------- next part --------------
#! /bin/sh
#
# Script to test tar files.
#

expected_fails=3
fails=0

run()
{
  echo "$*"
  $*
  if [ $? -ne 0 ]; then
    fails=$(expr $fails + 1)
  fi
}

header()
{
  echo "======================================="
  echo "$*"
  echo "---------------------------------------"
}

header "Test 1 set up"
run rm -rf x
run mkdir -p x/1
run tar cf t1.tar x
run rm -rf x
run mkdir x
run touch x/1
run tar cf t2.tar x
header "Change an empty directory to a file ..."
run rm -rf x
run tar xf t1.tar
run tar xf t2.tar
header "Change file to empty directory ..."
run rm -rf x
run tar xf t2.tar
run tar xf t1.tar

header "Test 2 set up"
run rm -rf x
run mkdir -p x/1
run touch x/1/1
run tar cf t1.tar x
run rm -rf x
run mkdir x
run touch x/1
run tar cf t2.tar x
header "Change directory to a file ..."
run rm -rf x
run tar xf t1.tar
run tar xf t2.tar
header "Change file to directory ..."
run rm -rf x
run tar xf t2.tar
run tar xf t1.tar

header "Test 3 set up"
run rm -rf x
run mkdir -p x/1
run touch x/2
run tar cf t1.tar x
run rm -rf x
run mkdir -p x/2
run touch x/1
run tar cf t2.tar x
header "Swap empty directory and file ..."
run rm -rf x
run tar xf t1.tar
run tar xf t2.tar
run rm -rf x
run tar xf t2.tar
run tar xf t1.tar

header "Test 4 set up"
run rm -rf x
run mkdir -p x/1
run touch x/1/1 x/2
run tar cf t1.tar x
run rm -rf x
run mkdir -p x/2
run touch x/2/1 x/1
run tar cf t2.tar x
header "Swap directory and file ..."
run rm -rf x
run tar xf t1.tar
run tar xf t2.tar
run rm -rf x
run tar xf t2.tar
run tar xf t1.tar

header "Total failures: $fails of $expected_fails"