Re: Untar pending changes - Was: Cutting 4.11.0 Soon

[Chris Johns]

On 20/11/2015 12:32 PM, Pavel Pisa wrote:
> Hello Chris,
> 
> thanks for clarification and test.
> 
> On Friday 20 of November 2015 00:51:44 Chris Johns wrote:
>> On 20/11/2015 2:56 AM, Pavel Pisa wrote:
>>> Hello Chris,
>>>
>>> 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?
>>
> 
> My patch solves only case where directory is "overwritten" by directory.
> As you have showed in the test script there are more legitimate cases
> (GNU tar on Linux seems to provide same results as your script expects).
> 

Yes. My intention is for us to review what is practical for our systems.

> It seems that usual/standardized tar behavior is to try unlink()
> if there is a non-directory object in a way of created directory.
> 
> A look at the full weight tar projects sources
> --
> 
> Because of licenses, I will point to BSD licensed libarchive now.
> That is code used by FreeBSD bsdtar version.
> 
> The source location of tar entry extract call
> 
>   https://github.com/libarchive/libarchive/blob/master/tar/read.c#L372
> 
> implementation of archive_read_extract2()
> 
>   
> https://github.com/libarchive/libarchive/blob/master/libarchive/archive_read_extract2.c#L83
> 
> for operations targetting real filesystem/disc results in a call to 
> _archive_write_disk_header()
> 
>   
> https://github.com/libarchive/libarchive/blob/master/libarchive/archive_write_disk_posix.c#L449
> 
> actual object is prepared in restore_entry() which is there
> 
>   
> https://github.com/libarchive/libarchive/blob/master/libarchive/archive_write_disk_posix.c#L1837
> 
> It seems to use unconditional unlink() if mode is ARCHIVE_EXTRACT_UNLINK is 
> set.
> For other modes it directly calls create_filesystem_object() which for 
> regular file does
> 
>   open(a->name, O_WRONLY | O_CREAT | O_EXCL | O_BINARY | O_CLOEXEC, mode)
> 
> for directories
> 
>   mkdir(a->name, mode);
> 
> If the first attempt fails with ENOTDIR or ENOENT, it tries to create 
> required directory structure.
> If error is EISDIR or EEXIST and mode is ARCHIVE_EXTRACT_NO_OVERWRITE, then 
> reports error.
> If the object in a way of regular file or other non-directory object is 
> directory (EISDIR),
> it tries rmdir() which should work on empty directory. If EEXIST is reported 
> then logic
> tries to find way to get rid of object. If original object is not directory 
> it uses unlink.
> If original object is a directory and extracted one is not, tries rmdir(). 
> Then tries to create
> object again.
> 
> If both, original and extracted objects are directory, then it skips deletion
> and notes that mode has to be updated later.

Thanks for the summary.

> Generally, use of libarchive directly in RTEMS would be most generic option.
> But for small tatgets (LPC17xx, small SPARCs etc.) it is a code and memory 
> killer
> which would demand more memory only for untar that deices has at all.

I agree libarchive is much to much for us. We need something lean and
small because tar files are often used to bootstrap a file systems.

> So it would worth to use simpler and smaller code. It would worth to unite
> three implementations on single one with pointer to function for read of
> source and function/description how to deal with target. There seems to be
> only one special case for now and it is extraction of regular file in IMFS.

Yes it would be good. A consistent result would make life simpler for
our users. Do we need a new ticket for this?

> 
> But such rewrite is not 4.11 material at all.
> 

I agree.

> If we consider rtems_filesystem_location_free() equivalent for both unlink() 
> and rmdir(),
> then directory to file switch case should be supported by actual 
> rtems_tarfs_load().
> The case of dir to dir is solved by patch already. Case of non dir to dir 
> needs additional
> unlink() and mkdir() attempt. This is reasonably simple and I can add it to 
> actual
> proposed code.

Excellent and thanks.

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: 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
#! /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 "---"
}

Re: Untar pending changes - Was: Cutting 4.11.0 Soon

[Pavel Pisa]

Hello Chris,

thanks for clarification and test.

On Friday 20 of November 2015 00:51:44 Chris Johns wrote:
> On 20/11/2015 2:56 AM, Pavel Pisa wrote:
> > Hello Chris,
> >
> > 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?
>

My patch solves only case where directory is "overwritten" by directory.
As you have showed in the test script there are more legitimate cases
(GNU tar on Linux seems to provide same results as your script expects).

It seems that usual/standardized tar behavior is to try unlink()
if there is a non-directory object in a way of created directory.

A look at the full weight tar projects sources
--

Because of licenses, I will point to BSD licensed libarchive now.
That is code used by FreeBSD bsdtar version.

The source location of tar entry extract call

  https://github.com/libarchive/libarchive/blob/master/tar/read.c#L372

implementation of archive_read_extract2()

  
https://github.com/libarchive/libarchive/blob/master/libarchive/archive_read_extract2.c#L83

for operations targetting real filesystem/disc results in a call to 
_archive_write_disk_header()

  
https://github.com/libarchive/libarchive/blob/master/libarchive/archive_write_disk_posix.c#L449

actual object is prepared in restore_entry() which is there

  
https://github.com/libarchive/libarchive/blob/master/libarchive/archive_write_disk_posix.c#L1837

It seems to use unconditional unlink() if mode is ARCHIVE_EXTRACT_UNLINK is set.
For other modes it directly calls create_filesystem_object() which for regular 
file does

  open(a->name, O_WRONLY | O_CREAT | O_EXCL | O_BINARY | O_CLOEXEC, mode)

for directories

  mkdir(a->name, mode);

If the first attempt fails with ENOTDIR or ENOENT, it tries to create required 
directory structure.
If error is EISDIR or EEXIST and mode is ARCHIVE_EXTRACT_NO_OVERWRITE, then 
reports error.
If the object in a way of regular file or other non-directory object is 
directory (EISDIR),
it tries rmdir() which should work on empty directory. If EEXIST is reported 
then logic
tries to find way to get rid of object. If original object is not directory it 
uses unlink.
If original object is a directory and extracted one is not, tries rmdir(). Then 
tries to create
object again.

If both, original and extracted objects are directory, then it skips deletion
and notes that mode has to be updated later.

Generally, use of libarchive directly in RTEMS would be most generic option.
But for small tatgets (LPC17xx, small SPARCs etc.) it is a code and memory 
killer
which would demand more memory only for untar that deices has at all.

So it would worth to use simpler and smaller code. It would worth to unite
three implementations on single one with pointer to function for read of
source and function/description how to deal with target. There seems to be
only one special case for now and it is extraction of regular file in IMFS.

But such rewrite is not 4.11 material at all.

If we consider rtems_filesystem_location_free() equivalent for both unlink() 
and rmdir(),
then directory to file switch case should be supported by actual 
rtems_tarfs_load().
The case of dir to dir is solved by patch already. Case of non dir to dir needs 
additional
unlink() and mkdir() attempt. This is reasonably simple and I can add it to 
actual
proposed code.

Best wishes,

  Pavel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: Untar pending changes - Was: Cutting 4.11.0 Soon

[Pavel Pisa]

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

Actual situation is that Untar_FromFile() ignores mkdir() error.
Untar_FromMemory() and rtems_tarfs_load() fails if there are directory
or file in a way.

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

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

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.
Problem to create symlink is reported.

rtems_tarfs_load() can target only IMFS and from data stored in memory
which stay constant for whole life of FS.
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.

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.

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

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

Thanks,

   Pavel



___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: Cutting 4.11.0 Soon

[Gedare Bloom]

nevermind, I pushed those through. they may have a bit of clean-up left though

On Wed, Nov 18, 2015 at 10:13 AM, Gedare Bloom  wrote:
> Pavel's patches for TMS570 are pending I think..
>
> On Tue, Nov 17, 2015 at 8:49 PM, Joel Sherrill  wrote:
>> Hi
>>
>> Now that the ftp site is cleaned uo, I want to cut this release as soon as
>> possible. Unfortunately, I have commitments and most likely won't get to
>> this before at least Friday.
>>
>> But caution also says I need to throw out a call for any pending patches
>> that should go in.
>>
>> Please speak up or I will timeout soon and start cutting the release.
>>
>> Thanks.
>>
>> --joel
>>
>>
>> ___
>> devel mailing list
>> devel@rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: Cutting 4.11.0 Soon

[Gedare Bloom]

Pavel's patches for TMS570 are pending I think..

On Tue, Nov 17, 2015 at 8:49 PM, Joel Sherrill  wrote:
> Hi
>
> Now that the ftp site is cleaned uo, I want to cut this release as soon as
> possible. Unfortunately, I have commitments and most likely won't get to
> this before at least Friday.
>
> But caution also says I need to throw out a call for any pending patches
> that should go in.
>
> Please speak up or I will timeout soon and start cutting the release.
>
> Thanks.
>
> --joel
>
>
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: Cutting 4.11.0 Soon

[Pavel Pisa]

Hello Gedare,

Premek has prepared update per your review remarks.
Change in pinmux and SCI has been quickly tested.

If you like the cleanup, please, push it to git.

We run more complete testing on Friday
but generally we can solve possible minor
problems and enhancements after 4.11 branching.

Complete loader support and startup code is
long term goal/wish which does not fit to 4.11
in any case.

Thanks,

   Pavel

On Wednesday 18 of November 2015 16:20:37 Gedare Bloom wrote:
> nevermind, I pushed those through. they may have a bit of clean-up left
> though
>
> On Wed, Nov 18, 2015 at 10:13 AM, Gedare Bloom  wrote:
> > Pavel's patches for TMS570 are pending I think..
> >
> > On Tue, Nov 17, 2015 at 8:49 PM, Joel Sherrill  wrote:
> >> Hi
> >>
> >> Now that the ftp site is cleaned uo, I want to cut this release as soon
> >> as possible. Unfortunately, I have commitments and most likely won't get
> >> to this before at least Friday.
> >>
> >> But caution also says I need to throw out a call for any pending patches
> >> that should go in.
> >>
> >> Please speak up or I will timeout soon and start cutting the release.
> >>
> >> Thanks.
> >>
> >> --joel
> >>
> >>
> >> ___
> >> devel mailing list
> >> devel@rtems.org
> >> http://lists.rtems.org/mailman/listinfo/devel
>
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: Cutting 4.11.0 Soon

[Chris Johns]

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
better as stray files do not hang around. I would rather we handle the
special cases correctly or not at all.

I have updated the ticket with this information.

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: Cutting 4.11.0 Soon

[Pavel Pisa]

Hello Joel, Gedare and others,

one patch to consider is the TAR fix

Untar_FromMemory breaks on create directory if they exists, even on root one.

https://devel.rtems.org/ticket/2413

A patch is attached.

But I have failed to find time to prepare correctly
integrated minimal test case.

The patch is important for unpacking standard tar command generated
archives used for example by some of Microwindows tests.

I am not sure if use of stat command could make some applications
larger or its linking can cause other problem. As for it invocation,
it is used only in case where original code lead to untar
fail.

Best wishes,

   Pavel

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Cutting 4.11.0 Soon

[Joel Sherrill]

Hi

Now that the ftp site is cleaned uo, I want to cut this release as soon as
possible. Unfortunately, I have commitments and most likely won't get to
this before at least Friday.

But caution also says I need to throw out a call for any pending patches
that should go in.

Please speak up or I will timeout soon and start cutting the release.

Thanks.

--joel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel